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

New CLI functionalities: tree, report, job info #180

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

gpetretto
Copy link
Contributor

Some new functionalities for the CLI:

  • To hopefully help new users explore the functionalities of the CLI, I have added a jf tree command. This prints a tree with the all the commands available. Can optionally control the depth of the tree, choose the starting point, show also the options for each command and the docstring for each branch of the tree. I thought it may be useful to have a quick bird-eye view if one is searching a specific command.
  • A new functionality to get a summary report of the state of the Jobs and Flows in the DB. Modeled on the report from Fireworks, can summarize the current number of jobs in different states and trends over time. Commands: jf job report, jf flow report.
  • Previously jf job info printed the content of JobInfo (or JobDoc) in alphabetical order. Now the order is predetermined, keeping close entried that are related to each other (e.g. all the dates).

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 91.74312% with 27 lines in your changes missing coverage. Please review.

Project coverage is 71.70%. Comparing base (8df1979) to head (de5b98a).

Files with missing lines Patch % Lines
src/jobflow_remote/cli/utils.py 74.41% 6 Missing and 5 partials ⚠️
src/jobflow_remote/jobs/jobcontroller.py 90.00% 3 Missing and 2 partials ⚠️
src/jobflow_remote/cli/formatting.py 95.83% 2 Missing and 2 partials ⚠️
src/jobflow_remote/cli/jf.py 81.25% 2 Missing and 1 partial ⚠️
src/jobflow_remote/jobs/report.py 97.61% 1 Missing and 1 partial ⚠️
src/jobflow_remote/utils/data.py 88.23% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           develop     #180       +/-   ##
============================================
+ Coverage    47.92%   71.70%   +23.78%     
============================================
  Files           43       44        +1     
  Lines         5156     5475      +319     
  Branches      1118     1191       +73     
============================================
+ Hits          2471     3926     +1455     
+ Misses        2424     1241     -1183     
- Partials       261      308       +47     
Files with missing lines Coverage Δ
src/jobflow_remote/cli/flow.py 72.11% <100.00%> (+48.95%) ⬆️
src/jobflow_remote/cli/job.py 82.68% <100.00%> (+62.24%) ⬆️
src/jobflow_remote/jobs/report.py 97.61% <97.61%> (ø)
src/jobflow_remote/utils/data.py 65.16% <88.23%> (+6.83%) ⬆️
src/jobflow_remote/cli/jf.py 84.78% <81.25%> (+10.58%) ⬆️
src/jobflow_remote/cli/formatting.py 85.82% <95.83%> (+75.04%) ⬆️
src/jobflow_remote/jobs/jobcontroller.py 78.35% <90.00%> (+42.54%) ⬆️
src/jobflow_remote/cli/utils.py 65.49% <74.41%> (+33.54%) ⬆️

... and 17 files with indirect coverage changes

@gpetretto
Copy link
Contributor Author

Also fixed the coverage report. From a few tests it seemed that running with --cov-append was not really working. Basically just sending to codecov the report from the integration tests. Sorry @ml-evs, do you see any issue with the changes in.github/workflows/testing.yml?

@ml-evs
Copy link
Member

ml-evs commented Sep 17, 2024

Also fixed the coverage report. From a few tests it seemed that running with --cov-append was not really working. Basically just sending to codecov the report from the integration tests. Sorry @ml-evs, do you see any issue with the changes in.github/workflows/testing.yml?

Hi @gpetretto, no issue from my side (especially as the number has gone up!) I'm surprised its not working given that the config was just taken from a project that does the same thing successfully, the only difference I can see is that the flag --cov=./optimade specifies a directory in my case, rather than a package name (though both should be supported). I can quickly open a PR for my own curiosity that checks if this is the issue, otherwise your approach looks robust to me!

Do you want me to take a look at the rest of the PR at all? I'm still fighting with SGE in #160...

@gpetretto
Copy link
Contributor Author

Hi @gpetretto, no issue from my side (especially as the number has gone up!) I'm surprised its not working given that the config was just taken from a project that does the same thing successfully, the only difference I can see is that the flag --cov=./optimade specifies a directory in my case, rather than a package name (though both should be supported). I can quickly open a PR for my own curiosity that checks if this is the issue, otherwise your approach looks robust to me!

Do you want me to take a look at the rest of the PR at all? I'm still fighting with SGE in #160...

Thanks for the tests on the coverage. For the review, I am not sure if @davidwaroquiers already started looking at this.

@davidwaroquiers
Copy link
Member

davidwaroquiers commented Sep 17, 2024

Thanks a lot for this! Not sure I'll have the time to review this soon enough (most probably not before next week). I could still already test the report, which is really nice! My only comments right now maybe would be about the tree command. I like the idea because we've had many people saying "oh, the jf command line interface has many options and it's difficult to find your way in it". That definitely helps! Would it be possible to put a small description of each of the commands also on the right (could it be taken from the help of each of the commands ? maybe it's too long? or maybe it would be too much anyway ?). My other comment is actually about having as a command itself. Wouldn't it be "better" to have it as an option ? e.g. jf --tree prints the entire tree, jf job --tree prints the tree starting at jf job, etc ... What do you think ?

@gpetretto
Copy link
Contributor Author

Thanks a lot for this! Not sure I'll have the time to review this soon enough (most probably not before next week). I could still already test the report, which is really nice! My only comments right now maybe would be about the tree command. I like the idea because we've had many people saying "oh, the jf command line interface has many options and it's difficult to find your way in it". That definitely helps! Would it be possible to put a small description of each of the commands also on the right (could it be taken from the help of each of the commands ? maybe it's too long? or maybe it would be too much anyway ?). My other comment is actually about having as a command itself. Wouldn't it be "better" to have it as an option ? e.g. jf --tree prints the entire tree, jf job --tree prints the tree starting at jf job, etc ... What do you think ?

For the first point, this should be already addressed. If you run jf tree -D you should also get the docstrings for all the entries that are shown.

Making it as an option instead of a command will be somewhat trickier to implement, but not too much. I may need to manually create a function for each of the subcommands. It should be easier to have it as an additional command for all the subcommands. e.g. jf tree, jf job tree, jf job set tree. I considered this option, but then it means that when you plot the full tree, you get all the tree commands in it, one for each subcommand, which makes it less readable.
If you think the --tree option is right way to go it can be implemented though. Maybe there is a way to add it to all the subcommands.

@davidwaroquiers
Copy link
Member

davidwaroquiers commented Sep 18, 2024

Or directly in the help ? Is there a verbosity for help ?

-h -v could give the tree for example

@gpetretto
Copy link
Contributor Author

Tthe --help option is generated automatically by typer. I think it may be possible to override it and complement it with the -v, but I don't think it will be any faster than the --tree. Probably more involved.

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.

4 participants