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

Implement importable config object in experimental API submodule #868

Merged
merged 46 commits into from
Dec 8, 2020

Conversation

stefsmeets
Copy link
Contributor

@stefsmeets stefsmeets commented Nov 20, 2020

Hi all, we started working on an importable config object. This PR can be used to track the progress. For more discussion see this issue: #498

This PR is a second attempt at implementing an importable config object, since #785 became 'too big to review'.

For this PR, I have kept the implementation completely separate from the current codebase in the future directory. In that case we can move faster, as the API will be somewhat experimentaland likely to change. Any useful bits can easily be moved down once we start to depen.

  • To start, I have added an importable config. A key feature is that all variables are validated. That means that unknown input will be rejected, and wrong values are easy to catch.
  • The config is strictly meant for interactive use at the moment.
  • I have made config-user valid by default. At present, users will have to update the config-user in order to do any work.
  • If .esmvaltool/config-user.yml does not exist, it will default to the one from the codebase.
  • Unlike the previous PR, the goal of this PR is NOT to change any config parameters. That will be done in a follow-up PR if still deemed necessary.

For a demonstration of the implementation, see this notebook here: https://gist.github.com/stefsmeets/dccd59d8c1f11768a9a5f297d7faeaab

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • Write documentation

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Described by #498
Successor of #785
Documentation: https://esmvaltool--868.org.readthedocs.build/projects/ESMValCore/en/868/api/esmvalcore.api.html

In Py3.8 it is OK to use `lru_cache` without parentheses, so
Py3.7: `@lru_cache()`, Py3.8: `@lru_cache`
@stefsmeets
Copy link
Contributor Author

stefsmeets commented Nov 23, 2020

Some feedback that is still relevant from @Peter9192 from #785 :

  • Should we expose all the methods of the cfg object? E.g. clear. I used this, and now I cannot get the default cfg back. Why would someone want to use pop or popitem? And what is the use case for find_all? I suppose it would be better to add things later than to have to remove them again.

Answer: The rest of esmvalcore expects the config to be like a dict, so we inherit from MutableMapping to maintan compatibility. That means pop / popitem / clear must be defined (https://docs.python.org/3/library/collections.abc.html). I don't know what else you expected when running .clear, but we can add some way to get the CFG back 😅

  • Related: I can't help but feeling that some of the config options are really very specific runtime arguments. If the session is very tightly coupled to a specific run, would it make sense to pass those runtime options only to the session, and not to the config?

Answer: Agreed. What you say makes sense, and it something that is already possible. However, ESMValCore depends on having some of these available in some places in the code. I would rather not mess with the config variables yet in this PR.

@stefsmeets stefsmeets marked this pull request as ready for review November 23, 2020 15:52
Copy link
Member

@nielsdrost nielsdrost left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @stefsmeets!

Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Like 👍

@Peter9192
Copy link
Contributor

One more thing: maybe we can print (on import?) that this is still in development and subject to frequent changes?

This will prevent undefined behaviour, because some values
must be checked/validated which is now done in main, which may not
always happen
@bouweandela
Copy link
Member

Would it be an idea to call the new module esmvalcore.experimental instead of esmvalcore.future? That's what they call it in iris too and I think it covers the content a bit better because possibly not everything from this module will move to the main tool.

@bouweandela
Copy link
Member

It would also be great if you could have a look at the Codacy warnings

By default, drs and rootpath are not defined, so we must have some
mechanism to warn users early that their config is not complete.
@stefsmeets
Copy link
Contributor Author

stefsmeets commented Dec 7, 2020

Regarding the exceptions and hiding the stack trace, the idea would be to hide the stack trace from standard out (and if applicable from main_log.txt, but not from main_log_debug.txt) if a well-known error occurs, so the user gets an easy to understand error message. At the moment this code raises many different exceptions, it would be good to reduce this to a single exception called e.g. ConfigError that inherits from the SuppressedError that we can then hide the stack trace of.

This works well... although that said, IPython uses a different mechanism altogether 🤦‍♂️
But... it works as expected in Jupyter 🎉

@stefsmeets stefsmeets changed the title Implement importable config object revisited Implement importable config object in experimental API submodule Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Notebook API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants