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

refactor/status #431

Closed
wants to merge 76 commits into from
Closed

refactor/status #431

wants to merge 76 commits into from

Conversation

bgunnar5
Copy link
Member

@bgunnar5 bgunnar5 commented Jul 12, 2023

This is the branch with all of the status command changes that I've been working on this year. I'm making this a draft PR for now so that we can get the review process started since it's a huge PR. I'm still working on finishing the documentation, and I still need to add tests.

This branch:

  • Introduces a new merlin queue-info command
    • this contains the same behavior as the previous status command
    • some additional behavior as well (active queue viewing, specific queue viewing, etc.)
  • Refactors the current merlin status command
    • now shows a step-by-step summary of the status of a study
    • includes a progress bar and a summary block for each step
    • additional options to remove prompts and add colorblind assistance
  • Introduces a new merlin detailed-status command
    • shows task-by-task statuses of a study
    • allows for filtering by return codes, task queues, task statuses, and workers
    • can set a limit on how many tasks to display
    • integrates Maestro's status display format and provides options to modify this display

There are still a couple of tickets for this that I have yet to implement but I figured they could be done after we get this released so users can start trying it out. Those tickets include:

  • Replacing MERLIN_FINISHED files with reads from MERLIN_STATUS.json files
  • Creating new status entries for each restart/retry
    • currently we're just counting the number of restarts which isn't super useful
    • I recently made the change from .txt files to .json files to help make this feature easier to implement
  • Condensing detailed-status output a bit more (I'm still planning out how to do this)
    • right now the output will show every single task and the info associated
    • maybe we can put tasks with similar statuses together?
      • e.g. all tasks with MERLIN_SUCCESS return code can be grouped together
    • maybe we don't need to show elapsed/run times and number of restarts for tasks grouped together?

@bgunnar5
Copy link
Member Author

I think it'd take some modification in the workspace method of the MerlinStudy class but it could be done

@bgunnar5
Copy link
Member Author

@lucpeterson @koning @doutriaux1 @jwhite242 I'd like your opinions here. I wrote a new unit test file called test_celeryadapter.py in order to test the functions that exist in the celeryadapter.py module (specifically the ones related to the merlin queue-info command for this branch). A lot of the functions in this module require an existing Celery app and, by association, an active Redis server. Therefore, writing tests for these functions required me to spin up a dummy Celery app and local Redis server. Since these tests depend on outside systems (Celery/Redis), would it be best to move them over to the integration test suite even though they're testing singular functions?

The GitHub action is failing at the moment since I'm starting a Redis server using the redis-server command, which it doesn't have access to. If I move these tests over to the integration test suite this would no longer be required, but I'm not sure if we should be testing singular functions within the integration test suite or not. What do you guys think?

@koning
Copy link
Member

koning commented Aug 15, 2023

Maybe these tests can be rolled into the merlin server tests, since that is starting a server?

@bgunnar5
Copy link
Member Author

That's not a bad idea. I'll try spinning the server up in the base class with the merlin server command instead of the redis-server one which should resolve the issue with the github action. Thanks for the suggestion

@bgunnar5 bgunnar5 mentioned this pull request Sep 26, 2023
@bgunnar5 bgunnar5 mentioned this pull request Oct 10, 2023
@bgunnar5 bgunnar5 closed this Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants