WIP: Add the ability to package more libraries #2269
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
romain-intel
force-pushed
the
feat/extended_packaging
branch
from
15e6e31 to
7b4c66f
Compare
This was referenced Feb 13, 2025
romain-intel
force-pushed
the
feat/extended_packaging
branch
from
7b4c66f to
789e841
Compare
romain-intel
force-pushed
the
feat/extended_packaging
branch
from
b27ab32 to
f665237
Compare
romain-intel
force-pushed
the
feat/extended_packaging
branch
from
f665237 to
c8a8af3
Compare
savingoyal
force-pushed
the
feat/extended_packaging
branch
from
e33972c to
8a3aabc
Compare
saikonen
reviewed
May 23, 2025
metaflow/meta_files.py
Outdated
There was a problem hiding this comment.
the uv_environment now depends on read_info_file so the import needs to be updated. Be sure to rebase this at some point to catch such errors.
| os.path.join(self.metaflow_dir.name, os.path.basename(INFO_FILE)), | ||
| os.path.join( | ||
| self.metaflow_dir.name, | ||
| os.path.basename(MetaFile.INFO_FILE.value), |
There was a problem hiding this comment.
MetaFile.INFO_FILE.value is just a filename so os.path.basename here seems unnecessary
| @@ -162,6 +161,8 @@ def get_username(): | |||
|
|
|||
|
|
|||
| def resolve_identity_as_tuple(): | |||
| from metaflow.exception import MetaflowUnknownUser | |||
There was a problem hiding this comment.
was moving these imports necessary? no objections, just wondering.
madhur-ob
force-pushed
the
feat/extended_packaging
branch
from
8a3aabc to
51323cf
Compare
madhur-ob
reviewed
May 23, 2025
metaflow/cmd/develop/stubs.py
Outdated
| if _py_ver >= (3, 8): | ||
| from importlib import metadata | ||
| elif _py_ver >= (3, 7): | ||
| from metaflow._vendor.v3_7 import importlib_metadata as metadata |
There was a problem hiding this comment.
earlier, it was from metaflow._vendor import importlib_metadata as metadata
but we do have a separate folder for v3_7
thus, I changed the import to:
from metaflow._vendor.v3_7 import importlib_metadata as metadata
madhur-ob
reviewed
May 23, 2025
| if _py_ver >= (3, 8): | ||
| from importlib import metadata | ||
| elif _py_ver >= (3, 7): | ||
| from metaflow._vendor.v3_7 import importlib_metadata as metadata |
There was a problem hiding this comment.
same comment as before:
earlier, it was from metaflow._vendor import importlib_metadata as metadata
but we do have a separate folder for v3_7
thus, I changed the import to:
from metaflow._vendor.v3_7 import importlib_metadata as metadata
Now allows: @Environment(vars=config_expr("...")) - This was broken since vars was a ConfigValue and not an updatable dictionary @project(name=config_expr("get_my_name(cfg)")) - This was broken because when evaluating the config_expr, we did not consider the globals at the point of call but instead inside the file defining config_expr. Tests are deployed internally.
- fixes a case where configs and Runner/Deployer were not compatible due to
configs not being processed in the same cwd
- fixes an issue with constructing the config values (introduced in
previous commit)
- fixes the error message when both default and specified config files
are not found
Previously something like:
RETRIES = config_expr("settings").retry
class MyFlow(FlowSpec):
@retries(times=RETRIES)
@step
def start(self):
...
@retries(times=RETRIES)
@step
def end(self):
...
would fail. Also, something like:
FOO = config_expr("settings").foo.bar
BAR = FOO.BAZ
BLAH = FOO.BAZ2
would also fail.
This fixes these issues by copying DelayEvaluator everytime and also
making the `__call__` function reentrant.
Two types of decorators are supported:
- a "pure" Python decorator that allows you to execute something before
and after a step. You also have the possibility of entirely skipping a step
(which is a fairly requested feature)
- mutators (Flow level or Step level). These mutators allow you to modify
the flow/steps dynamically. This is particularly useful when put together
with configs.
Still needs quite a few tests but it should be workable. A few features to try out:
- You should be able to add mutators on a base flow spec class and have it
work in a derived class
- the add_decorator function on the MutableStep should take strings,
actual MF decorators and user decorators
- the add_decorator function on the MutableFlow should take strings
and actual flow level decorators.
- --with should work with any step level decorator (user defined including
step mutators):
- the name is the FQN for the class. It supports passing arguments as well
- you can also use a shorter name provided that name is available *somewhere* where
the flowspec is defined (either itself or one of its base classes)
- mix and match decorators should work
- the UserStepDecorator class has a init function that can take arguments
if you want to build something a bit more fancy
- the mutators also take arguments
- config values can be used for any of those arguments (and of course can be
used inside any of those functions)
romain-intel
force-pushed
the
feat/extended_packaging
branch
from
51323cf to
98545b5
Compare
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.