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

Use importlib as the import mode for pytest #3672

Merged
merged 5 commits into from
Jun 24, 2024

Conversation

ehogan
Copy link
Contributor

@ehogan ehogan commented Jun 21, 2024

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:

@ehogan ehogan self-assigned this Jun 21, 2024
@ehogan ehogan added this to the v2.11.0 milestone Jun 21, 2024
@ehogan ehogan added the testing label Jun 21, 2024
@ehogan ehogan marked this pull request as ready for review June 21, 2024 08:46
@bouweandela
Copy link
Member

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

@bouweandela
Copy link
Member

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
Copy link
Contributor Author

ehogan commented Jun 21, 2024

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 😊

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

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 🍺

@valeriupredoi valeriupredoi mentioned this pull request Jun 21, 2024
12 tasks
@bouweandela bouweandela merged commit b946c9b into main Jun 24, 2024
8 checks passed
@bouweandela bouweandela deleted the 3670_use_pytest_importlib branch June 24, 2024 16:24
@ehogan
Copy link
Contributor Author

ehogan commented Jun 25, 2024

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix failing tests after CMIP6 climate patterns merge
3 participants