Tickets/dm 46151 #20
Tickets/dm 46151 #20
Conversation
…le table of a couple columns, add night filter
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
| @@ -0,0 +1,271 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #2. f' from {min_day_obs} to {date}. ')
I think for our initial investigating we were allowing many days and different days, etc, but I think when possible/relevant we should shift the focus more towards 'what happened last night' and allow that to be the default assumption of what users would be interested in. yagni
This comment applies to record_limit, number_of_days, and this message
Reply via ReviewNB
There was a problem hiding this comment.
Without number_of_days greater than one, users may find it difficult to see if results look good. If they happen to have a day with no results (happens on usdf-dev frequently) the only remedy would be to try another day and see if that works. Repeat a few times and its frustrating. In this case to pick one day, we have to have a min and max day so we already have to have a number of days concept that finds its way into code. After everyone agrees on the format of displays, we can remove the number_of_days parameter from sidecar yaml file so user will always get the default of 1.
| @@ -0,0 +1,271 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. # Paste contents of source_adapters.py in new cell below this one.
Are you planning on having a different PR to implement that so it works in Times Square, or should we do this now?
Reply via ReviewNB
There was a problem hiding this comment.
Argg, it should be done now! Thanks!!
| @@ -0,0 +1,271 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #5. recs = service_adapter.get_messages(limit=limit,
I think we should change both instances of recs to something more descriptive, and differ the narrative from the exposure one. e.g. exposure_records/messages narrative_records/entries
Reply via ReviewNB
There was a problem hiding this comment.
Agreed. Its a even worse than the obvious because exposure has that we might want to show for both "messages" and "exposures".
| @@ -1,303 +1,50 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #1. status = Dashboard().report(timeout=0.5)
Why add the dashboard as separate to the NightLog? At least the way I had pictured it, we would first scroll the nightlog, see all the places we are actively able to query, and then below that see the data we have from those sources.
I am open to differently, but just curious why you chose this way.
Reply via ReviewNB
There was a problem hiding this comment.
Because it cannot work. The dashboard accesses several different environments (different host servers). Those other environments are generally not accessible (not authorized) from inside RSP on one environment.
There was a problem hiding this comment.
I thought that was the point of showing the dashboard of sources, to show oops cannot access xxx-source
Maybe I am misunderstanding the dashboard as a whole?
There was a problem hiding this comment.
You are. It tells about access for each source endpoint on each server. I've had troubles with one endpoint works on one but not the other. Need to know that earlier.
There was a problem hiding this comment.
Let's add this topic to the agenda tomorrow?
| @@ -0,0 +1,755 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #55. self.ignore_fields = list()
I was curious about whether or not to use list() or [] after seeing these lines, and it looks like performance wise we should use the latter
https://stackoverflow.com/questions/5790860/whats-the-difference-between-and-vs-list-and-dict
Reply via ReviewNB
There was a problem hiding this comment.
I find it hard to read (see) the [] method. I think its the list() method makes it more obvious that my intent is to create a list that is always empty at that point. I think its more readable. I don't think performance of creating an empty list matters (if its even true). Your link talks more about time of dict().
| @@ -0,0 +1,755 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #64. to = (timeout or self.timeout)
I suggest timeout = but also, what happens if they are both set, the first entry overrules? Maybe we should prefer timeout = max(timeout, self.timeout) to select the larger of the two?
Reply via ReviewNB
| @@ -0,0 +1,755 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #67. url_http_status_code = dict()
see comment about list() versus [] for dict() and {} as well please
Reply via ReviewNB
| @@ -0,0 +1,755 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #71. r = requests.get(url, timeout=(timeout or self.timeout))
use what is set in #64
Reply via ReviewNB
|
I added a few comments on the pasted Adapters, you do not have to implement them here, but keep them in mind for follow on PRs please. You have otherwise handled the other questions and comments I had. Thank you |
Combines sources into NightlyLog. Parameterized by day_obs and number_of_days.