ESMValGroup / ESMValTool

ESMValTool: A community diagnostic and performance metrics tool for routine evaluation of Earth system models in CMIP
https://www.esmvaltool.org
Apache License 2.0
210 stars 121 forks source link

Use `importlib` as the import mode for `pytest` #3672

Closed ehogan closed 1 week ago

ehogan commented 1 week ago

Description

Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the ๐Ÿ›  Technical or ๐Ÿงช Scientific review.


To help with the number of pull requests:

bouweandela commented 1 week ago

Are you sure all the imports work like this? I'm a bit concerned about the missing __init__.py files in many directories.

bouweandela commented 1 week ago

On the other hand, the tests are passing, so it's probably fine. What's your opinion? Add __init__.py files to be on the safe side or leave it as is?

ehogan commented 1 week ago

On the other hand, the tests are passing, so it's probably fine. What's your opinion? Add __init__.py files to be on the safe side or leave it as is?

As I understand it, __init__.py files shouldn't be needed when running pytest when using the importlib mode.

I have just run the recipe_ensclus.yml and recipe_iht_toa.yml recipes at the MO (because I know we have the data for these recipes!) using this branch and they both succeed, so these changes do work when executed ๐Ÿ’ช

I would prefer not to add lots of __init__.py files, if we can help it, from a maintenance perspective ๐Ÿ˜Š

ehogan commented 1 week ago

Package-level imports are very good and very welcome, Emma, cheers! Note that Bouwe and me are doing that in #3646 since we added an __all__ member in the init, plus, this is the way to import anyway. I would, however, support Bouwe's idea to add init files - they don't need maintenance, since they be empty ๐Ÿบ

Apologies for not replying to this sooner! I did some quick research into __init__.py modules and discovered that in Python 3.3 a new type of package was introduced (alongside regular packages); namespace packages, see Python documentation: Modules: Packages. Namespace packages have no __init__.py module. PEP 420 provides information about implicit namespace packages.

I haven't yet figured out the advantages and disadvantages for each approach / which is prefered / whether they can be used together, but it might be good to understand this before deciding what approach to take ๐Ÿ‘

bouweandela commented 1 week ago

I think it will be simplest if we make ESMValTool a single package. At the moment, due to missing __init__.py files, various subdirectories are interpreted as separate (namespace) packages. But we can address that later, as long as it works for now.