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

Feature/optional dependencies #269

Merged
merged 21 commits into from
Mar 15, 2024
Merged

Conversation

sandorkertesz
Copy link
Collaborator

@sandorkertesz sandorkertesz commented Dec 4, 2023

Fixes #252

See docs at: https://earthkit-data--269.org.readthedocs.build/en/269/install.html

The optional Python dependencies are now controlled in setup.cfg.

By default when installed from PyPI we get a minimal version:

image

Optional dependencies can be installed as:

image

@sandorkertesz sandorkertesz linked an issue Dec 4, 2023 that may be closed by this pull request
@sandorkertesz sandorkertesz marked this pull request as draft December 4, 2023 16:04
@sandorkertesz sandorkertesz marked this pull request as ready for review March 14, 2024 13:27
@sandorkertesz sandorkertesz changed the title WIP: Feature/optional dependencies Feature/optional dependencies Mar 14, 2024
@sandorkertesz
Copy link
Collaborator Author

Some more questions:

  • should we make "grib" or "netcdf" as an optional dependency?
  • how should earthkit and earthkit-maps define earthkit-data as a dependency? Should they specify earthkit-data[all] ???
  • how should the earthkit-data conda install behave?

Copy link
Member

@iainrussell iainrussell 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! I see we mention eccodes twice as a dependency (generally, and for BUFR support). Is that because BUFR support might require a newer version? I'd say that since both instances of the eccodes dep currently require the same version, we could drop it as a separate dep for BUFR.

@sandorkertesz
Copy link
Collaborator Author

Looks good to me! I see we mention eccodes twice as a dependency (generally, and for BUFR support). Is that because BUFR support might require a newer version? I'd say that since both instances of the eccodes dep currently require the same version, we could drop it as a separate dep for BUFR.

Fixed now!

@iainrussell
Copy link
Member

Great, I think we're ready to merge then. Nice work!

@sandorkertesz
Copy link
Collaborator Author

Great, I think we're ready to merge then. Nice work!

Thank you! What do you think about the questions above?

@iainrussell
Copy link
Member

iainrussell commented Mar 15, 2024

Some more questions:

Sorry, I did not notice those questions.

  • should we make "grib" or "netcdf" as an optional dependency?

I think for now they should be always there, as they are 'core' data types that we support, and the dependencies are easy to install everywhere.

  • how should earthkit and earthkit-maps define earthkit-data as a dependency? Should they specify earthkit-data[all] ???

Good question. Probably 'earthkit' should install all, and ek-maps should be more conservative, but this is a slightly different question from this specific PR. And it depends a bit on whether lacking the binary dependencies would cause an error during 'import earthkit.data' in the 'all' case.

  • how should the earthkit-data conda install behave?

Probably it should use 'all', but this will not work (yet) in practice because we do not have all the binaries available on conda-forge (e.g. odc, fdb). Once we have our own conda channel up and running, we will be able to do this. So, we should just put all the dependencies that are available on conda-forge as dependencies of the conda-forge package.

@sandorkertesz
Copy link
Collaborator Author

  • how should earthkit and earthkit-maps define earthkit-data as a dependency? Should they specify earthkit-data[all] ???

Good question. Probably 'earthkit' should install all, and ek-maps should be more conservative, but this is a slightly different question from this specific PR. And it depends a bit on whether lacking the binary dependencies would cause an error during 'import earthkit.data' in the 'all' case.

'import earthkit.data' should be fine even when the binary dependencies are missing. This PR is only addressing the Python dependencies.

@sandorkertesz sandorkertesz merged commit c8445e7 into develop Mar 15, 2024
59 checks passed
@sandorkertesz sandorkertesz deleted the feature/optional-dependencies branch March 15, 2024 14:43
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.

Make some dependencies optional
2 participants