Conversation
no has_umap, just return lib or not as test
| ), "minibatches emb and X must have same number of rows since h(df) = emb" | ||
| df = df.assign(x=emb.x, y=emb.y) # add x and y to df for graphistry instance | ||
| try: | ||
| df = df.assign(x=emb.x, y=emb.y) # add x and y to df for graphistry instance |
There was a problem hiding this comment.
instead of try/catch, can we do value inspection?
| try: | ||
| dist = np.linalg.norm(diff, axis=1) # Euclidean distance | ||
| except TypeError: | ||
| dist = np.linalg.norm(diff.to_pandas(), axis=1) # Euclidean distance |
There was a problem hiding this comment.
same, instead of try/catch, can we do value inspection?
There was a problem hiding this comment.
@lmeyerov I will back this out everywhere (not too pervasive in overhaul yet, 10 files including tests), but its pretty simple, works well, and gets rid of instances like in embed_utils where you end up using the lazy imports almost comically throughout, so i can put this into a seperate PR, only to be reunited one day far off
_, torch, _, _, _, _, _, _ = lazy_embed_import_dep()
_, _, _, dgl, _, _, _, _ = lazy_embed_import_dep()
| def __init__(self): | ||
| self.pkgs = {} | ||
|
|
||
| def __getattr__(self, pkg:str): |
There was a problem hiding this comment.
instead call this def import(...) or something to make it explicit that code is running
| @@ -0,0 +1,30 @@ | |||
| import importlib | |||
There was a problem hiding this comment.
from importlib import __import__, import_module ?
| @@ -0,0 +1,30 @@ | |||
| import importlib | |||
|
|
|||
| class DepManager: | |||
There was a problem hiding this comment.
move dep_manager.py to utils/dep_manager.py
|
|
||
| def import_from(self, pkg:str, name:str): | ||
| try: | ||
| module = __import__(pkg, fromlist=[name]) |
There was a problem hiding this comment.
importlib docs recommend using importlib.import_module instead
| @@ -0,0 +1,30 @@ | |||
| import importlib | |||
|
|
|||
There was a problem hiding this comment.
this class benefits from a comment on its design and the problem it solves
There was a problem hiding this comment.
ex: maybe it does a fast & cached check of a package being in the path before actually importing, so there should be a testable and observable speedup?
| try: | ||
| return self.pkgs[pkg] | ||
| except KeyError: | ||
| return None |
There was a problem hiding this comment.
instead of hiding the exception from the user, it's probably more informative and unsurprising to:
- cache the exception during
_add_deps - fetch the cached exception, instead of None, and rethrow here, instead of return
|
Plan is we'll split this into 2 PRs, one on depman (see comments) and a separate on featurE_uitls fixes |
3 participants
to test gpu-featurization (cu_cat) with new dependency manager