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

Enh: Adding model package download functions to loads #55

Merged
merged 16 commits into from
Oct 5, 2023

Conversation

NickGeneva
Copy link
Collaborator

@NickGeneva NickGeneva commented Oct 3, 2023

Earth-2 MIP Pull Request

Description

Fixes: #54

This PR does a few items, however the primary objective is to enable auto-downloading of model packages that are known but are not present on the disk. This PR:

  • Adds package download functions into each of the three known models, must be triggered with the prefix e2mip:// on the model name
  • Add loader functions for just Pangu_6 and Pangu_24hr
  • Adds support for using earth2mip entry points to discover load functions. This means people can add their own model via a python package that injects itself into earth2mips entry point group earth2mip.networks. This eliminates the requirement for metadata.json in favor of pure python. This is how the known models are discovered.

Now running a ensemble config without a any of these models in the model registry just works.

Notebooks could be updated but they do provide good info about how these packages are set up. Should probably get rid of the sub-processes to wget commands to pythonic process. But one step at a time.

Open questions: How should we test this?

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The CHANGELOG.md is up to date with these changes.
  • An issue is linked to this pull request.

Dependencies

@NickGeneva NickGeneva added the enhancement New feature or request label Oct 3, 2023
@NickGeneva NickGeneva self-assigned this Oct 3, 2023
@NickGeneva
Copy link
Collaborator Author

/blossom-ci

@NickGeneva NickGeneva marked this pull request as ready for review October 3, 2023 23:59
@NickGeneva NickGeneva added the 3 - Ready for Review Ready for review by team label Oct 4, 2023
Copy link
Collaborator

@nbren12 nbren12 left a comment

Choose a reason for hiding this comment

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

Thanks. Users will indeed find this very useful, but I am not comfortable with the amount of I/O specific concerns in the packages, or with the backwards incompatible changes to Package.__init__.

Package should abstract such concerns and hide them from the load functions. I think a possible usage could be this:

package = Package("ngc://nvidia/modulus/modulus_dlwp_cubesphere/v0.1:zip")
earth2mip.networks.dlwp(package)

I'll have to think more about the entrypoint mechanism for defining model metadata. For CLI use, we already have a similar mechanism here:

def add_model_args(parser: argparse.ArgumentParser, required=False):
. This is used by the lagged ensembles script. For API use, I'm not sure the auto-discovery is needed.

earth2mip/networks/__init__.py Outdated Show resolved Hide resolved
earth2mip/networks/fcnv2_sm.py Outdated Show resolved Hide resolved
earth2mip/networks/fcnv2_sm.py Outdated Show resolved Hide resolved
earth2mip/networks/fcnv2_sm.py Outdated Show resolved Hide resolved
earth2mip/networks/fcnv2_sm.py Outdated Show resolved Hide resolved
earth2mip/networks/fcnv2_sm.py Outdated Show resolved Hide resolved
earth2mip/model_registry.py Outdated Show resolved Hide resolved
earth2mip/networks/dlwp.py Outdated Show resolved Hide resolved
earth2mip/networks/dlwp.py Outdated Show resolved Hide resolved
earth2mip/networks/pangu.py Outdated Show resolved Hide resolved
@NickGeneva
Copy link
Collaborator Author

/blossom-ci

@NickGeneva
Copy link
Collaborator Author

/blossom-ci

@NickGeneva
Copy link
Collaborator Author

/blossom-ci

@NickGeneva
Copy link
Collaborator Author

/blossom-ci

@NickGeneva
Copy link
Collaborator Author

/blossom-ci

@NickGeneva
Copy link
Collaborator Author

/blossom-ci

Copy link
Collaborator

@nbren12 nbren12 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. Just a couple minor comments, but I am hitting approve. Feel free to merge after addressing them. I don't think @yairchn needs to review this.

setup.py Outdated Show resolved Hide resolved
examples/README.md Outdated Show resolved Hide resolved
@NickGeneva
Copy link
Collaborator Author

/blossom-ci

@NickGeneva NickGeneva merged commit c674c8d into NVIDIA:main Oct 5, 2023
1 check passed
@NickGeneva NickGeneva deleted the ngeneva/modeldl branch November 22, 2023 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀[FEA]: Add download function to known models
2 participants