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.

Sign up for GitHub

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

Jump to bottom

PREOPS-5505 Restructure schedview snapshot dashboard code #103

Merged
merged 23 commits into from
Sep 23, 2024
Merged

PREOPS-5505 Restructure schedview snapshot dashboard code #103

merged 23 commits into from
Sep 23, 2024

Conversation

emanehab99
Copy link
Collaborator

  • Reorganise the dashoboard code into separate modules
  • Rename modules to be more comprehensive
  • Add more comments to explain how the dashboard is updated using Panel
  • Clean code and amend docstrings to follow development guidelines

@emanehab99 emanehab99 self-assigned this Aug 27, 2024
ehneilsen
ehneilsen approved these changes Sep 6, 2024
View reviewed changes
Copy link
Collaborator

@ehneilsen ehneilsen left a comment

Choose a reason for hiding this comment

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

I had just a few minor suggestions, and isort/ruff seems to be complaining. Go ahead and merge after addressing these.

schedview/app/scheduler_dashboard/constants.py Outdated Show resolved Hide resolved
schedview/app/scheduler_dashboard/utils.py Outdated Show resolved Hide resolved
schedview/app/scheduler_dashboard/scheduler_dashboard_app.py
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <https://www.gnu.org/licenses/>.

"""schedview docstring"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put an actual docstring here.

Copy link
Collaborator Author

@emanehab99 emanehab99 Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mind if you provide the docstring @ehneilsen ? I think it will be more comprehensive and clear in your own words

schedview/app/scheduler_dashboard/unrestricted_scheduler_snapshot_dashboard.py Outdated Show resolved Hide resolved
@emanehab99
Copy link
Collaborator Author

@ehneilsen I can see that the isort/ruff issues are all in the jupyter notebooks. They seem to be there in previous PR's as well.

@emanehab99
Copy link
Collaborator Author

The tests were passing before but now test_scheduler is failing, also failing in main. Looks like it's an issue with rubin_scheduler and the sample_pickle. Would you like me to merge anyway @ehneilsen ?

@ehneilsen
Copy link
Collaborator

There were some changes in dependencies (rubin_scheduler) that required updates to schedview. These are now merged onto main, so try rebasing your changes and see if the tests pass. Changes to main should also have made ruff happy. If things pass after rebasing, go ahead and merge.

@emanehab99
Copy link
Collaborator Author

Ruff tests pass now but the unit tests are still failing. They pass locally though!

@ehneilsen
Copy link
Collaborator

There've been recent changes to rubin_scheduler that required changes to schedview. If you update your local rubin_scheduler and rebase your changes to schedview, it should work and you should be testing with the same versions github is.

@emanehab99 emanehab99 merged commit 5cedc7d into main Sep 23, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehneilsen ehneilsen ehneilsen approved these changes

@alserene alserene Awaiting requested review from alserene

Assignees

@emanehab99 emanehab99

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emanehab99 @ehneilsen @alserene