Skip to content
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

Update the new document specifying working with public dashboard. #1056

Merged
merged 12 commits into from
Apr 20, 2024

Conversation

iantei
Copy link
Contributor

@iantei iantei commented Mar 12, 2024

No description provided.

@iantei iantei marked this pull request as ready for review March 12, 2024 20:13
Copy link
Member

@Abby-Wheelis Abby-Wheelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small suggestion on breaking up a bullet point but other than that looks great to me! I think this will be a great resource for people working on the public dashboard so we can easily find what we need to know for development and testing!

- Make sure the default charts are loaded at the first instance of loading the website.
- Make sure all the charts are being launched or the error table is displayed.
2. Test with loading and unloading different program/study dataset.
- Load different datasets, run the scripts to execute the notebooks to make sure the charts are being generate properly. Ensure to unload the datasets, and re-load the new dataset.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to put the step between loading in it's own bullet, like:

... make sure the charts are being generated properly

  • Between datasets, drop the old dataset and load the new dataset

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iantei thanks for submitting this - I always like to see new documentation!

However, I am very reluctant to duplicate code in the documentation unless we have somebody signed up to keep the documentation in sync.

Bad/obsolete documentation is just as bad as no documentation.
For concrete examples, see below.

Once you remove the duplicated code, I am happy to merge!

Comment on lines 104 to 110
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/update_mappings.py mapping_dictionaries.ipynb
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py generic_metrics.ipynb default
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py generic_metrics_sensed.ipynb default
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py generic_timeseries.ipynb default
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py mode_specific_metrics.ipynb default
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py mode_specific_timeseries.ipynb default
$ (emission) root@06f07def2c0e:/usr/src/app/saved-notebooks# PYTHONPATH=.. python bin/generate_plots.py energy_calculations.ipynb default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list of notebooks will become obsolete as soon as the stacked bar charts change is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed reference to all notebooks, while just keeping mapping_dictionaries.ipynb and generic_metrics. Added a note to reference notebooks from crontab in em-publicdashboard.

Comment on lines 75 to 81
services:
notebook-server:
environment:
- DB_HOST=mongodb://db/openpath_prod_usaid_laos_ev
- WEB_SERVER_HOST=0.0.0.0
- CRON_MODE=TRUE
- STUDY_CONFIG=usaid-laos-ev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may become obsolete once the changes discussed in #1048 are checked in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since, there's still progress ongoing for changes related to #1048 , I have included a note to update this once the changes re checked-in.

Copy link
Contributor

@shankari shankari Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1048 is being actively worked on, and will be merged in the next month or so. And the next time it is updated, this will need to be updated as well. I don't understand why this is so important to be replicated here.

As I said before:

Bad/obsolete documentation is just as bad as no documentation.

Why does this need to be replicated here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to be replicated here?

At present, while documenting the process of working with Public Dashboard, this is a rightful approach of changing the dataset and study config in the docker-compose file. Therefore, I felt it would be rightful to keep this part of documentation (provided reference that change would be need to be made when the #1048 is done). Considering document would be updated when changes are made on the the referred issue.

But accounting for the future prospect of being obsolete, the above block of instructions have been removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it is fine to showcase how to change the values. But you can omit the parts that have not changed - e.g.

        environment:
        - DB_HOST=mongodb://db/openpath_prod_usaid_laos_ev
        - ....
        - STUDY_CONFIG=usaid-laos-ev

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification, I have updated the section showcasing the example of how to change the values for DB_HOST and STUDY_CONFIG, while omitting the parts which have not been changed.

docs/dev/publicdashboard/Working_with_public_dashboard.md Outdated Show resolved Hide resolved
- Load different datasets, run the scripts to execute the notebooks to make sure the charts are being generate properly.
- Between datasets, drop the old dataset and load the new dataset.
- Also check with empty MongoDB.
3. After the code has been merged, validate the changes on the Staging.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: fix "the staging"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it to "the staging" since there were other required changes too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iantei this is a nit, but "the staging" is incorrect here. It should be "validate the changes on staging" or "validate the changes on the staging environment".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the final fix needed

docs/dev/publicdashboard/Working_with_public_dashboard.md Outdated Show resolved Hide resolved
docs/dev/publicdashboard/Working_with_public_dashboard.md Outdated Show resolved Hide resolved
docs/dev/publicdashboard/Working_with_public_dashboard.md Outdated Show resolved Hide resolved
docs/dev/publicdashboard/Working_with_public_dashboard.md Outdated Show resolved Hide resolved
@shankari
Copy link
Contributor

One final fix

@shankari shankari merged commit 64c7563 into e-mission:master Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants