-
Notifications
You must be signed in to change notification settings - Fork 808
feature: deferred loading and requirement pruning #1199
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
Changes from 96 commits
3000d4c
757e0f3
9310d0a
dac569e
35e93fc
bf7f36b
6a39b0c
56c6182
3657e04
865d604
d61957d
8a7051e
60775f6
31e98d4
75babb7
83f551a
dd51196
b33a46c
de5b3f1
19c31fe
54fabc5
ffac714
97c8160
1d4e69c
6164bc5
85fb7c3
0402116
e287fe9
6339648
76b1774
ca133e4
8e8a5b9
aa7500a
4f2e5ef
69cfef2
a1da5ed
3a8605d
d2d17ad
13974b8
dc83929
d527650
8c46730
06180b6
7c22dea
ce23d70
8ab94bd
bb67a3e
b45ba35
de86505
38e6a15
64399a5
65ac9fe
ea71cea
14b958d
d0c90ea
de28c75
1742f60
c877225
6eede53
2bcca2c
9d57b5c
e195148
f8e8635
246b7dc
b9e3b73
40b6bcc
ae5ad5d
1a57c8e
738e8e8
46e1794
dfbe9ac
35fa3d6
2e6054f
04b4731
f9138f5
4f3aeae
a35409f
e89b3f2
0f7a618
1f9998c
dafef3b
c2708c6
c5a95a9
b4b7504
0e074dc
c60d814
309a246
74ff9a3
677eacb
6da2389
27119e3
b06a634
07de920
143f84d
69244dc
53597f0
5daff3d
957cc75
621bfaf
0be265b
05b9f5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -404,6 +404,28 @@ def load_plugin(path, break_on_fail=True, config_root=_config) -> object: | |||||||||||||||||||||||||||||||||||
| ) from ve | ||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||
| return False | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| full_plugin_name = ".".join((category, module_name, plugin_class_name)) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| # check cache for optional imports | ||||||||||||||||||||||||||||||||||||
| if category in PLUGIN_TYPES: | ||||||||||||||||||||||||||||||||||||
| extra_dependency_names = PluginCache.instance()[category][full_plugin_name][ | ||||||||||||||||||||||||||||||||||||
| "extra_dependency_names" | ||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||
| if len(extra_dependency_names) > 0: | ||||||||||||||||||||||||||||||||||||
| absent_modules = [] | ||||||||||||||||||||||||||||||||||||
| for dependency_module_name in extra_dependency_names: | ||||||||||||||||||||||||||||||||||||
| for ( | ||||||||||||||||||||||||||||||||||||
| dependency_path | ||||||||||||||||||||||||||||||||||||
| ) in [ # support both plain names and also multi-point names e.g. langchain.llms | ||||||||||||||||||||||||||||||||||||
| ".".join(dependency_module_name.split(".")[: n + 1]) | ||||||||||||||||||||||||||||||||||||
| for n in range(dependency_module_name.count(".") + 1) | ||||||||||||||||||||||||||||||||||||
| ]: | ||||||||||||||||||||||||||||||||||||
| if importlib.util.find_spec(dependency_path) is None: | ||||||||||||||||||||||||||||||||||||
| absent_modules.append(dependency_module_name) | ||||||||||||||||||||||||||||||||||||
| if len(absent_modules): | ||||||||||||||||||||||||||||||||||||
| _import_failed(absent_modules, full_plugin_name) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
| if category in PLUGIN_TYPES: | |
| extra_dependency_names = PluginCache.instance()[category][full_plugin_name][ | |
| "extra_dependency_names" | |
| ] | |
| if len(extra_dependency_names) > 0: | |
| absent_modules = [] | |
| for dependency_module_name in extra_dependency_names: | |
| for ( | |
| dependency_path | |
| ) in [ # support both plain names and also multi-point names e.g. langchain.llms | |
| ".".join(dependency_module_name.split(".")[: n + 1]) | |
| for n in range(dependency_module_name.count(".") + 1) | |
| ]: | |
| if importlib.util.find_spec(dependency_path) is None: | |
| absent_modules.append(dependency_module_name) | |
| if len(absent_modules): | |
| _import_failed(absent_modules, full_plugin_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in tension with the requested feature of listing all missing modules at once rather than piecemeal? I realise the granularity is different, but don't we want to cause the minimum number of user round trips between execution and dep installation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, load_plugin is only called when actually instantiating a plugin, testing in this location we will not evaluate all plugins required for the run. Since generators are the primary plugin type using this pattern just removing this is acceptable for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should
- merge these threads
- get clear about reqs for this PR
I don't mind if we advise all missing modules at once or piecemeal. The latter has better UX. Agree this feature should belong in the right PR and code location, if the feature is going to manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, based on the targeted changes in scope this PR should remove this block.
Also the revisions already to made to load_deps will provide a consistent verbose list of all packages required for the plugin that is attempting to load. This covers the same scope as what this block does with added context available.
jmartin-tech marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting, it might be nice to have this reference a group in the pyproject.toml. This may be added in #1475 when the as these groups are introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing shows
find_specneeds to be provided the runtime package name, currentlydependency_pathentries are the pypi package name.This can be tested by attempting to load
generators.huggingface.LLaVA.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this code in particular, I don't think this validation is needed here at this time. It may be more appropriate have handle a missing import exception around the module instantiation call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the intent here is to summarise in one pass a list of all missing module names, which is determined using
find_spec.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is simply not the right place for this,
load_pluginis called at instantiation so this is only evaluating for this plugin which is fully processed by_load_depsso not needed here.In the next iteration PR #1475 we might want to add an early preprocessor that takes a comprehensive look at the full run config to determine if all dependencies required for the full run are available however I am thinking that might turn out to be an overly complex goal that may get deferred or shelved in favor of allowing the run to skip
probesthat happen to be missing dependencies instead for blocking start of the run. More discussion of that can happen in that PR in later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this was extra code added after initial PR was made when a feature was requested to list all missing modules rather than one at a time. Is that additional feature no longer a requirement to land?