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

Option for stored data in jf job list #228

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

Conversation

FabiPi3
Copy link
Collaborator

@FabiPi3 FabiPi3 commented Dec 10, 2024

Add the option to display data from the stored_data field in the Response object. A User can select a key or multiple keys to display those values.

On a sidenote, another option was added to not display the uuid of the jobs. I typically don't gain much from this information since the db_id already gives me the unique information I need to find the job in the database.

Things to discuss:

  • Name of the cli options (including short-cuts)
  • Name of the column in the output table: For now only the key. Should be sufficient since the user does know what fields he/she requested, right?
  • Name of the default_value which is displayed once no data is available or the specified key is not present. For now a light grey italic none

Example:

See the attached screenshot.

Bildschirmfoto 2024-12-10 um 11 32 32

Maybe closes #227

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 91.48936% with 4 lines in your changes missing coverage. Please review.

Project coverage is 73.15%. Comparing base (41bf0b2) to head (c3904e8).

Files with missing lines Patch % Lines
src/jobflow_remote/cli/formatting.py 94.73% 1 Missing and 1 partial ⚠️
src/jobflow_remote/cli/job.py 71.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #228      +/-   ##
===========================================
- Coverage    73.18%   73.15%   -0.03%     
===========================================
  Files           48       48              
  Lines         6347     6348       +1     
  Branches      1012     1013       +1     
===========================================
- Hits          4645     4644       -1     
- Misses        1341     1342       +1     
- Partials       361      362       +1     
Files with missing lines Coverage Δ
src/jobflow_remote/cli/flow.py 70.00% <ø> (ø)
src/jobflow_remote/config/settings.py 92.30% <100.00%> (+0.30%) ⬆️
src/jobflow_remote/jobs/data.py 88.98% <100.00%> (+0.04%) ⬆️
src/jobflow_remote/cli/formatting.py 80.35% <94.73%> (-0.48%) ⬇️
src/jobflow_remote/cli/job.py 81.91% <71.42%> (-0.32%) ⬇️

@gpetretto
Copy link
Contributor

Hi @FabiPi3,

sorry for the delay, but I did not have much time to dedicate to jobflow-remote in the last days. I think the option for the stored_data is a good update.
Concerning the job id, I think it would be good to introduce a better customization of the displayed result, rather than just an option to skip one value. Otherwise we could end up with a very large number of options to control each displayed fields. I think it would be interesting to introduce a --format/--output option, to control the columns printed. With the additional option to directly customize this in the ~/.jfremote.yaml configuration file. What do you think about this solution? And would you be interested/available to try implementing it? In that case I can give some more tips about the code that needs to be modified. No pressure anyway in case you would rather not work more on this point.

In any case, for all the features that will be added I will also ask you to implement a test in https://github.com/Matgenix/jobflow-remote/blob/develop/tests/db/cli/test_job.py before merging.

@FabiPi3
Copy link
Collaborator Author

FabiPi3 commented Dec 19, 2024

I think you should upgrade the minimal typer version. I have troubles running the tests with typer 0.9.0, but newer versions does work. In the CI for python 3.12 typer version 0.12.something is used.

@FabiPi3
Copy link
Collaborator Author

FabiPi3 commented Dec 19, 2024

I have added an option to explicitly specify all columns that will be displayed. This can also be set in the config file. I had to rewrite the formatting table function to make it more dynamic. Let me know what you think.

I have also added a small test, would that be sufficient?

I was just wondering about to how to specify the columns in the terminal. Right now I use the list option from typer, which can be quite lengthy to type: jf -fe job list -m 3 -hk db_id -hk name -hk state -hk worker -hk locked -hk last_updated -hk run_time

Maybe is would be better to just pass a string: jf job list -hk db_id&name&state&worker and than split this string afterward. What do you think?

Copy link
Contributor

@gpetretto gpetretto left a comment

Choose a reason for hiding this comment

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

Hi @FabiPi3,

thanks a lot for implementing the additional options! I think the implementation is fine. I left some minor comments on the code.
I agree with you that repeating the option multiple times is really inconvenient for this kind of field. I think it would be good to switch to a single string, as you suggested. But I would use a , as a separator, rather than a &. It seems more standard and usually easier to type on the keyboard.

The test is fine, but can you also add one where it fails by passing an option that does not exist? You can pass error=True to run_check_cli and check that (part of) the error message is present in the output.
Also, if you add the error for incompatible options and that -v overrides the configuration, it would be good to have tests for those use cases as well.

src/jobflow_remote/cli/formatting.py Outdated Show resolved Hide resolved
src/jobflow_remote/cli/job.py Outdated Show resolved Hide resolved
src/jobflow_remote/cli/job.py Show resolved Hide resolved
src/jobflow_remote/config/settings.py Outdated Show resolved Hide resolved
src/jobflow_remote/config/settings.py Outdated Show resolved Hide resolved
src/jobflow_remote/cli/job.py Outdated Show resolved Hide resolved
@FabiPi3
Copy link
Collaborator Author

FabiPi3 commented Dec 22, 2024

Hi Guido,

thanks for your feedback. I tried implementing all of it. I have also changed my code a bit.

I added a unit to the run time field as it felt necessary to me. What do you think?

Regarding the tests, I added two failing tests. I am not sure how to modify the SETTINGS object for the test to test this. Do you have a standard procedure?

@gpetretto
Copy link
Contributor

Thanks for the updates.
For the test with changing the SETTINGS you can do it as in this test:

m.setattr(SETTINGS, "projects_folder", os.getcwd())

Aside from this and adjusting the description of cli_job_list_columns, I think it will be ready to be merged.
I see that the tests are failing, but it seems unrelated to your changes and it is probably due to an inaccurate tests based on the dates of the job report.

@FabiPi3
Copy link
Collaborator Author

FabiPi3 commented Dec 24, 2024

Added your comments. I agree, the failing tests seemed unrelated to my changes.

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.

Check status for simulation code in jf job list
3 participants