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

Enable running without a config file #30

Closed
greschd opened this issue Mar 12, 2020 · 6 comments · Fixed by #33
Closed

Enable running without a config file #30

greschd opened this issue Mar 12, 2020 · 6 comments · Fixed by #33

Comments

@greschd
Copy link
Member

greschd commented Mar 12, 2020

Currently, the .aiida-testing-config.yml config file is committed to the repository. In general (and for plugins), this kind of defeats the purpose of the config file: It contains the parts that are different on different systems, and can not be hardcoded in the tests.

A partial solution is to just recommend not committing .aiida-testing-config.yml, however it would be good to make all fixtures run (if all caches are present) also without a config file, for the purposes of CI. Having to create a special "CI config file" instead would be quite cumbersome.

@greschd greschd added this to the Version 0.1 release milestone Mar 12, 2020
@greschd greschd changed the title Committing the config file / run on CI Enable running without a config file Mar 12, 2020
@ltalirz
Copy link
Member

ltalirz commented Mar 12, 2020

Indeed - ideally I would prefer not having a config file at all, but I don't really see a viable way of avoiding it.

I would propose the following approach for aiida plugins using aiida-testing:

  • don't commit the .aiida-testing-config.yml
  • ideally, running the tests for the first time would create somehow a template configuration file that one just needs to copy into .aiida-testing-config.yml. Otherwise one needs to make sure to document the shape of the config file for every plugin with the needed code labels (also possible)
  • it definitely needs to be possible to run the tests without a config file being present (as it's not actually needed unless one needs to regenerate test data)

For CI, I don't quite get the issue. I think it's fine to assume that you regenerate test data locally, and CI tests will be using the test data already present.

@greschd
Copy link
Member Author

greschd commented Mar 12, 2020

Yeah, CI was just my use case for "being able to run without config file".

@greschd
Copy link
Member Author

greschd commented Mar 12, 2020

* ideally, running the tests for the first time would create somehow a template configuration file that one just needs to copy into `.aiida-testing-config.yml`. Otherwise one needs to make sure to document the shape of the config file for every plugin with the needed code labels (also possible)

Hmm, for the mock codes maybe when trying to run a code that has no entry it just populates the config file with <label>: null.
It's not quite clear to me how to find the place where the config file should be put, though. Maybe the "automatic empty config file" should be behind a pytest flag? If we just do it automatically, all of the considerations w.r.t. overwriting existing config files, backup, etc. comes into play again.

@ltalirz
Copy link
Member

ltalirz commented Mar 12, 2020

It's not quite clear to me how to find the place where the config file should be put, though.

I guess this question of the config file location applies just as well to config loading (#12 ).
If we provide a way to specify the search path, it should apply to both.
Otherwise, I would say the CWD is also fine for the moment.

Maybe the "automatic empty config file" should be behind a pytest flag? If we just do it automatically, all of the considerations w.r.t. overwriting existing config files, backup, etc. comes into play again.

I agree that overwriting an existing config file automatically is not a good idea.
If it's easy enough to add a flag --generate-config (analogous to jupyter ) to pytest, that would be great.

There was still some more uncertainty hidden in the "somehow" of my comment - how is the creation of the template going to work?
Does the mock_code_factory (with the flag enabled) add new labels to the config file?

@greschd
Copy link
Member Author

greschd commented Mar 12, 2020

I guess this question of the config file location applies just as well to config loading (#12 ).
If we provide a way to specify the search path, it should apply to both.
Otherwise, I would say the CWD is also fine for the moment.

It's not quite the same though, because with an existing config you can upwards-search from CWD, making sure to always hit it as long as you're in the repo (if you put it at the root level).

But if we make --generate-config a flag, I think CWD is fine -- move it afterwards if really needed.

I agree that overwriting an existing config file automatically is not a good idea.
If it's easy enough to add a flag --generate-config (analogous to jupyter ) to pytest, that would be great.

Yeah, I agree that this is the way to go — since pytest flags is a limited namespace the flag name probably needs to be something like --generate-aiida-testing-config, though.

There was still some more uncertainty hidden in the "somehow" of my comment - how is the creation of the template going to work?
Does the mock_code_factory (with the flag enabled) add new labels to the config file?

Yes, I think this would be the way to go - obviously it will only create the it for codes that are actually used in the test run, but there doesn't appear to be a way around that.

A point to consider is that the configuration generation should work for the different parts of aiida-testing, not only mock codes — so we need a common (internal) API for updating the config file, which is then used by the different parts.

@ltalirz
Copy link
Member

ltalirz commented Mar 12, 2020

since pytest flags is a limited namespace the flag name probably needs to be something like --generate-aiida-testing-config, though.

Sounds good!

All the rest seems more or less agreed. Now we just need to implement it ;-)

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 a pull request may close this issue.

2 participants