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

Add the verdi profile caching command #4245

Closed

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jul 10, 2020

Fixes #4240

This command will try to load the caching configuration from the caching
configuration file, for any given profile. This is useful for users to
make sure that the file is correctly named and contains correct syntax,
before actually starting to run new calculations.

To test this, a new pytest fixture is created that creates a completely
new and independent configuration folder, along with fixtures to create
profiles to add to the config and caching configuration files. Similar
code already exists for normal unittests, but this can be removed once
those have been refactored to pytests.

@sphuber sphuber force-pushed the feature/4240/verdi-caching-config branch from 8fef65f to 4ad43ad Compare July 10, 2020 12:12
@sphuber sphuber requested a review from greschd July 10, 2020 12:13
@sphuber
Copy link
Contributor Author

sphuber commented Jul 10, 2020

Note that this is missing documentation, but I will add this once PR #4228 has been merged.

@sphuber sphuber force-pushed the feature/4240/verdi-caching-config branch from 4ad43ad to 7725d6f Compare July 10, 2020 16:28
@sphuber sphuber requested a review from ltalirz July 10, 2020 16:35
@ltalirz
Copy link
Member

ltalirz commented Jul 10, 2020

Thanks @sphuber !

Two questions from browsing through quickly would be:

  • why not include the caching configuration in the output of verdi profile show as we are anyhow planning to merge the caching configuration into the configuration file?
  • for the new fixtures for creating profiles: can we try to reuse somehow this context manager?
    def temporary_config_instance():

I'll give a proper review next week

@sphuber
Copy link
Contributor Author

sphuber commented Jul 10, 2020

* why not include the caching configuration in the output of `verdi profile show` as we are anyhow planning to merge the caching configuration into the configuration file?

My thinking is that at some point we want to add an endpoint to also modify the caching from the interface. I though of verdi config but its interface doesn't support what we'd need for the config, so I thought the best place was verdi profile caching. Glad to have input if you think of a better place. Didn't think it deserved a top-level command group in any case. Could be wrong though.

* for the new fixtures for creating profiles: can we try to reuse somehow this context manager?
  https://github.com/aiidateam/aiida-core/blob/e4c7fb51c44cb260c4f983f3bdd0a54688659901/tests/utils/configuration.py#L47

See the commit and PR message. The tests using it should actually be refactored to pytest in which case they should use this new fixture instead. Had already opened an issue for this refactoring.

I'll give a proper review next week

Thanks a lot!

@chrisjsewell
Copy link
Member

I though of verdi config but its interface doesn't support what we'd need for the config

I think ideally this should go in verdi config. As mentioned in #4260 (comment), I think we are arbitrarily limiting the config interface by trying to mirror exactly git config.

@sphuber sphuber force-pushed the feature/4240/verdi-caching-config branch 2 times, most recently from 4c87a09 to 9f68472 Compare July 15, 2020 09:44
@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #4245 into develop will increase coverage by 0.03%.
The diff coverage is 92.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4245      +/-   ##
===========================================
+ Coverage    79.18%   79.20%   +0.03%     
===========================================
  Files          468      468              
  Lines        34476    34513      +37     
===========================================
+ Hits         27295    27333      +38     
+ Misses        7181     7180       -1     
Flag Coverage Δ
#django 71.13% <92.00%> (+0.04%) ⬆️
#sqlalchemy 71.96% <92.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/manage/caching.py 98.12% <90.33%> (-1.17%) ⬇️
aiida/cmdline/commands/cmd_profile.py 87.70% <94.74%> (+4.72%) ⬆️
aiida/manage/configuration/config.py 92.66% <0.00%> (+0.57%) ⬆️
aiida/cmdline/utils/defaults.py 85.72% <0.00%> (+14.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67791f6...b363031. Read the comment docs.

This command will try to load the caching configuration from the caching
configuration file, for any given profile. This is useful for users to
make sure that the file is correctly named and contains correct syntax,
before actually starting to run new calculations.

To test this, a new pytest fixture is created that creates a completely
new and independent configuration folder, along with fixtures to create
profiles to add to the config and caching configuration files. Similar
code already exists for normal unittests, but this can be removed once
those have been refactored to pytests.
@sphuber sphuber force-pushed the feature/4240/verdi-caching-config branch from 9f68472 to b363031 Compare July 15, 2020 11:44
@ltalirz
Copy link
Member

ltalirz commented Jul 17, 2020

I agree with @chrisjsewell that it would be nice to come to an agreement on what the role of the verdi profile vs verdi config commands should be going forward (this also ties into #4243 ).

I could well imagine having verdi config as an API for modifying the config.json file (possibly starting just with things like caching etc.), while verdi profile as the commands that perform additional actions, such as populating the database with tables or creating the db in the first place.

@chrisjsewell
Copy link
Member

note I've made an issue for discussing verdi config (with a proposal) #4584

@chrisjsewell
Copy link
Member

chrisjsewell commented Feb 9, 2021

FYI this PR is now superceded by #4712 😜

@ltalirz
Copy link
Member

ltalirz commented Feb 9, 2021

Then let's close this

@ltalirz ltalirz closed this Feb 9, 2021
@sphuber sphuber deleted the feature/4240/verdi-caching-config branch April 28, 2021 12:04
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.

Add verdi command that parses and prints the caching configuration
3 participants