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

Nwm client transition #265

Merged
merged 7 commits into from
Nov 21, 2024
Merged

Nwm client transition #265

merged 7 commits into from
Nov 21, 2024

Conversation

jarq6c
Copy link
Collaborator

@jarq6c jarq6c commented Nov 20, 2024

This PR deprecates hydrotools.caches and hydrotools.nwm_client. Importing these packages results in a FutureWarning that these packages are no longer supported. I would have gone with a DeprecationWarning, but that is not shown to the user by default. Automated tests are removed, as are references in the docs.

@jarq6c
Copy link
Collaborator Author

jarq6c commented Nov 20, 2024

@aaraney any notes or reservations?

@jarq6c jarq6c self-assigned this Nov 20, 2024
@jarq6c jarq6c added the documentation Improvements or additions to documentation label Nov 20, 2024
@aaraney
Copy link
Member

aaraney commented Nov 20, 2024

Im sure we have talked about this idea in the past and my memory is failing me, @jarq6c what are your thoughts on instead of deprecating going with a hard break and replacing the current nwm_client with what is now the nwm_client_new and releasing a new major version of nwm_client?

@jarq6c
Copy link
Collaborator Author

jarq6c commented Nov 20, 2024

Im sure we have talked about this idea in the past and my memory is failing me, @jarq6c what are your thoughts on instead of deprecating going with a hard break and replacing the current nwm_client with what is now the nwm_client_new and releasing a new major version of nwm_client?

Definitely preferred. Do you have any recommendations for how to do that responsibly?

@jarq6c
Copy link
Collaborator Author

jarq6c commented Nov 21, 2024

I'm going to rename nwm_client_new on another PR.

@jarq6c jarq6c merged commit 8e0e374 into NOAA-OWP:main Nov 21, 2024
@jarq6c jarq6c deleted the nwm-client-transition branch November 21, 2024 14:06
@aaraney
Copy link
Member

aaraney commented Nov 21, 2024

Definitely preferred. Do you have any recommendations for how to do that responsibly?

Ideally we would transition nwm_client_new to nwm_client and deprecate nwm_client_new but forward imports until we decide to officially pull nwm_client_new for good.

I need to explore a bit to see how to do this and cover all cases, but for starters I think the module level __getattr__ semantics added in PEP 562 enable "proxying" one module as if it were another.

@aaraney
Copy link
Member

aaraney commented Nov 25, 2024

It seems that PEP 562 does provide a means to proxy a package. However, it does seem that there is a fair amount of boilerplate and structural coupling. The below examples are the contents of a namespace package alt.sub that proxies imports to pkg.sub.

# alt/sub/__init__.py

def __getattr__(name: str):
    import importlib
    package_name = "pkg.sub"
    try:
        return importlib.import_module(f".{name!s}", package_name)
    except ImportError:
        mod = importlib.import_module(package_name)
        return getattr(mod, name)
# alt/sub/a.py
# NOTE: This needs to be repeated for **all** python modules in `pkg.sub`.

def __getattr__(name: str):
    import importlib
    package_name = "pkg.sub"
    proxy_name = f"{package_name}.{__name__[len(package_name)+1:]}"
    mod = importlib.import_module(proxy_name)
    return getattr(mod, name)

Take aways:

Im not sure that it is worth creating and maintaining this file structure to deprecate nwm_client_new. IMO I think we should cut a final release of nwm_client_new that warns users to move to nwm_client and provides instructions for installing and how to migrate (if that's even applicable).

@jarq6c jarq6c mentioned this pull request Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants