-
-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes v2 dev website dashboard not loading #1256
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the debugging!
If there's really no data for the given time range, we should display that one way or another instead of loading data from a different time range. Is that possible?
It maybe possible to put the problematic code into a callback function so that whenever there is no data, we use the PreventUpdate() method (which should be available in older versions of dash as well...?) to stop the graph content from rendering. From my understanding so far, the code breaks when there is no data because the pandas groupby method generates an empty dataframe, with columns that plotly fails to index. I will need to take a look at this again tomorrow. |
Gotcha. If it turns out that we can only fix this issue in the newer version of Dash, we can lump those together. |
@nichhk this will probably the only one we need to merge now. Will make a new PR for combine dashboards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some partial comments for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates!
Can you also add screenshots showing how the graphs look on this PR?
|
||
# Populate the neighborhood dropdown | ||
fig = px.line() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to have more descriptive variable names, e.g., requests_over_time_line_fig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a few more readability/style comments.
In the future, I think we would greatly benefit from having function docstrings so that we know exactly what the arguments and return types are. But we can work on that in a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for working on this! Please add screenshots of the dashboard when there is no data.
Still trying to work on that. Getting feedback is a slow process as dash/gunicorn is a little buggy - sometimes showing no data on the dashboard. Hot reloading is not that useful on this front as dashboards tend to be cached and takes some "waiting time" before it refreshes. Regardless, I will work on the function documentation before updating that. |
Ah gotcha, that sounds finicky. Eventually, we would want unit tests to test this sort of scenario for us. But for now, would it help to simply replace the call to the API here with something that just creates an empty dataframe, or something like that? |
There are too many dependencies in the code that relies on the table columns, so what I did was basically hav date range in the future such that there is no data. The problem is a "successful reload" (i.e. refresh with code changes) takes too long becuaes of either 1) buggy gunicorn/dash interaction causing no data (happens when none of the calllback function gets fired), or 2) dashboards are cached so it's still using old code. Multiple manual refresh is necessary to get 1 "successful reload". |
There seems to be some error popping up that I missed before. First line is "selected_council'" 's datatype, and second line is "start_date" 's datatype. Do you have any suggestions?
I also added the docstrings for the functions in nc_recent. Will continue working on this tmr. |
Hmm I'm not sure. It looks like |
Yeah the coercing seems to be the problem. The dataframe from the API gets one request with only created date and no close date (NaN) value, which the pd.to_datetime coerce as a float. The weird thing is that I have set the start date so far ahead of time and the API still return this request with a startdate close to today. I revert to your suggestion on using empty data frame. I also update the last row assignment in the line chart dataframe using the following code
If we simply use J, the same request type will be assigned on all date range (since the j value remains constant on the outer loop). But we want all request types to have value 0 in all date range. For the pie chart, turns out using value 0 would just cause the chart to disappear altogether. I use 1 in this case. Let me know what you think. EDIT: on the other hand, we do need to figure out a way to deal with dataframe with createdDates only and no closedDates (incomplete requests). Any thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Josh! Just a few comments about the docstring and then this should be good to go.
Thanks for the review. Updated docstrings with args and returns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience Josh! LGTM. Just a few comments. I approved the PR so that you can merge it after fixing the comments without waiting for another review from me.
Fixes #1251
Resolve typeName key error by reloading older data when filtered table is empty
dev
branchAny questions? See the getting started guide