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

[FEA] Smart dependency manager to replace lazy imports #468

Open
tanmoyio opened this issue Apr 18, 2023 · 3 comments · May be fixed by #511
Open

[FEA] Smart dependency manager to replace lazy imports #468

tanmoyio opened this issue Apr 18, 2023 · 3 comments · May be fixed by #511
Assignees

Comments

@tanmoyio
Copy link
Member

tanmoyio commented Apr 18, 2023

Motivations:

  • Sometimes modules are getting imported several times through lazy import
  • Not easy to manage dependencies from file to file

This is how we are managing dependencies now

# current state of lazy import 
def lazy_umap_import_has_dependancy():
    try:
        import warnings

        warnings.filterwarnings("ignore")
        import umap  # noqa

        return True, "ok", umap
    except ModuleNotFoundError as e:
        return False, e, None

After dependency manager it will look like

deps = DepManager()
cuml, has_cuml = deps.cuml
umap, has_umap = deps.umap

Aim

  • provides readability, smart dependency management
  • easy to write unit tests,
  • it will only import the packages once

working example

Screenshot 2023-04-18 at 9 50 45 PM

@lmeyerov
Copy link
Contributor

Better code reuse here seems good, and enables memoization etc tricks

Also, I think somewhere in the code I realized a useful trick was, when just a check is needed, instead of trying via an import (slow), do a check for the module in the path, which skips the costs of actually importing (which runs all sorts of code in the dep's top-level)

@tanmoyio tanmoyio self-assigned this May 15, 2023
@dcolinmorgan
Copy link
Contributor

dcolinmorgan commented Oct 6, 2023

is it cheating to use another library to help?

import importlib.util, sys
def check_umap():
  umap_spec = importlib.util.find_spec("umap")
  has_umap = umap_spec is not None
  if has_umap and "umap" not in sys.modules:
     import umap
     return umap, has_umap
  else:
     return None, has_umap

i see the way we do it now is better for >3.6, but since we want to check and not import every time perhaps if > try?

@lmeyerov
Copy link
Contributor

lmeyerov commented Oct 6, 2023

Hmm, these calls do different things;

  • importlib will see it is resolvable: This has false positives - still needs an 'import' w try/catch to truly validate - you can DOWNLOAD cudf on a sys without a GPU, just not import it. A downside is it might be a tad slower as a full search vs kv lookup.

  • sys.modules will check if it is already imported, but has false negatives in that it will miss existence when cudf is downloaded but the user hasn't imported elsewhere yet (I think?) . That is arguably a feature, but is not obvious to work with, so maybe a bug?

So yeah, maybe:

  • encapsulate
  • the True/False is unnecessary , return Tuple[Optional[package], Optional[exn]] ?
  • Hopefully mypy is ok with a pattern here..
  • find_spec. Changes behavior, but likely to match user intent better.
  • with caching decorator so won't search every single time

@dcolinmorgan dcolinmorgan linked a pull request Oct 10, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants