packaging #2438

madhur-ob · 2025-05-29T18:35:17Z

No description provided.

@Environment

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

@retries

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.

savingoyal · 2025-05-29T20:33:20Z

metaflow/client/core.py

-        # The JSON module in Python3 deals with Unicode. Tar gives bytes.
-        info_str = (
-            self._tar.extractfile(os.path.basename(INFO_FILE)).read().decode("utf-8")
+        info_str = MFEnv.get_archive_content(self._tar, MetaFile.INFO_FILE).decode(


this could be cleaned up better - MFEnv.from_archive(self._tar).get_info()

savingoyal · 2025-05-29T20:33:39Z

metaflow/decorators.py

@@ -14,18 +14,14 @@
 from .parameters import current_flow
 from .user_configs.config_decorators import CustomStepDecorator
 from .user_configs.config_parameters import (
+    ConfigValue,


savingoyal · 2025-05-29T20:34:08Z

metaflow/decorators.py

-    unicode
-except NameError:
-    unicode = str
-    basestring = str


we can pull out Python2 changes in a separate PR

THere are definitely other changes. We can revert this but I think we can leave this here for now and yes, do a separate PR to pull out all the other ones.

savingoyal · 2025-05-29T20:35:39Z

metaflow/decorators.py

+        tarball. This hook can return one of two things:
+          - a generator yielding a tuple of `(file_path, arcname)` to add files to
+            the code package. `file_path` is the path to the file on the local filesystem
+            and `arcname` is the path relative to the packaged code.
+          - a generator yielding a tuple of `(content, arcname, type)` where:
+            - type is a AddToPackageType
+            - for CODE_FILE:
+              - content: path to the file to include
+              - arcname: path relative to the code directory in the package
+            - for CODE_MODULE:
+              - content: name of the module
+              - arcame: None (ignored)
+            - for CONFIG_FILE:
+              - content: path to the file to include
+              - arcname: path relative to the config directory in the package
+            - for CONFIG_CONTENT:
+              - content: bytes to include
+              - arcname: path relative to the config directory in the package


this is quite funky - returning one of two things with different types. need to fix this

What do you suggest? As I mentioned:

we can make it a breaking change (change the signature of add_package)

we can add a different method (feels wonky too)

??

savingoyal · 2025-05-29T20:37:12Z

metaflow/meta_files.py

+# moving here. The reason is that this is needed to read extension information which needs
+# to happen before mfenv gets packaged.
+
+MFENV_DIR = (


change MFENV to MFCODE. we already have --environment and @Environment - MFENV will only lead to more confusion here and everywhere.

savingoyal · 2025-05-29T20:41:11Z

metaflow/meta_files.py

+        if version not in get_filname_map:
+            raise ValueError(
+                "Unsupported packaging version '%s'. Please update Metaflow" % version
+            )


will we actually ever hit this?

yes -- if a newer version of Metaflow has a newer packaging method and an old client is used to access stuff.

romain-intel · 2025-05-29T20:43:59Z

metaflow/decorators.py

-    except:
-        # python 2
-        f.name = f.__func__.func_name
+    f.wrappers = []


This is actually not related to this PR. It slipped through.

savingoyal · 2025-05-29T20:46:27Z

metaflow/meta_files.py

+    # here that is then used in MFEnv (it logically belongs in MFEnv but that file can't
+    # be loaded just yet).
+    global _info_file_content
+    global _info_file_present


do we need this extra global

_UNINITIALISED = object() # sentinel _info_file_content = _UNINITIALISED def read_info_file(): """Return the parsed info.json once, or None if the file is absent.""" global _info_file_content if _info_file_content is _UNINITIALISED: info_filename = get_filename(MetaFile.INFO_FILE) if info_filename is not None: with open(info_filename, "r", encoding="utf-8") as f: _info_file_content = json.load(f) else: _info_file_content = None return _info_file_content

I can make that change. Technically there is an extra global too (the _UNINITIALIZED) but it can be shared.

savingoyal · 2025-05-29T20:48:02Z

metaflow/meta_files.py

+class MetaFile(Enum):
+    INFO_FILE = "INFO"
+    CONFIG_FILE = "CONFIG_PARAMETERS"
+    INCLUDED_DIST_INFO = "INCLUDED_DIST_INFO"


what is INCLUDED_DIST_INFO - hard to understand what this implies - is there an EXCLUDED_DIST_INFO too somewhere?

I can clarify: it means the information about any distribution that is included. So no, there is no excluded one. I can just hame it DIST_INFO.

savingoyal · 2025-05-29T20:48:41Z

metaflow/meta_files.py

+)
+
+
+class MetaFile(Enum):


for future generations looking at the code, it will be immensely useful to call out in detail what is included in these files.

Sure. I can add this. We didn't add any file (except dist info) but can make it more clear as part of making the code better.

savingoyal · 2025-05-29T20:49:28Z

metaflow/meta_files.py

+)
+
+
+class MetaFile(Enum):


btw, do we need to have three separate files? any value in merging all/some?

We can definitely have them all as one. I vaguely remember you being in favor of separate files in the past but I have no strong preference.

savingoyal · 2025-05-29T20:50:43Z

metaflow/meta_files.py

+
+
+def generic_get_filename(
+    name: Union[MetaFile, str], is_meta: Optional[bool] = None


is_meta is not actually used. consider dropping

savingoyal · 2025-05-29T20:53:19Z

metaflow/meta_files.py

+def v1_get_filename(
+    name: Union[MetaFile, str],
+    meta_info: Dict[str, Any],
+    is_meta: Optional[bool] = None,


how does this work if i am interested in a file that unfortunately clashes with an internal reserved name? also, is_meta actually useful? what if we simply had another method that provided the contents of INFO file and it included everything you need?

perhaps by setting is_meta to false actively somewhere?

let me check on that. The idea of putting stuff in mfconf is that they won't clash with code.

savingoyal · 2025-05-29T20:56:03Z

metaflow/meta_files.py

+
+def read_included_dist_info():
+    global _included_dist_info
+    global _included_dist_present


same comment as above

savingoyal · 2025-05-29T21:00:00Z

metaflow/metaflow_environment.py

+        Called to add custom files needed for this environment. This hook will be
+        called in the `MetaflowPackage` class where metaflow compiles the code package
+        tarball. This hook can return one of two things:
+          - a generator yielding a tuple of `(file_path, arcname)` to add files to


this is a bit funky, we shouldn't return two types of tuples

already addressed above with a question.

savingoyal · 2025-05-29T21:03:02Z

metaflow/package/mfenv.py

+        return None
+
+
+class MFEnv:


can MetaflowPackage instead be the way that rest of the code accesses functionality here? will make it easier for all the decorator implementations which already have access to the package object to get access to info files etc.

reading through the code, MFEnv seems like an internal implementation detail for MetaflowPackage. If we can keep it as such throughout the code, that will be great!

I wouldn't say it's a implementation detail but more a part of it but yes, I can move some of those methods back out to MetaflowPackage. That is not a problem.

savingoyal · 2025-05-29T21:13:19Z

metaflow/plugins/pypi/conda_decorator.py

@@ -159,11 +159,11 @@ def runtime_init(self, flow, graph, package, run_id):
            os.path.join(self.metaflow_dir.name, "metaflow"),
        )

-        info = os.path.join(get_metaflow_root(), os.path.basename(INFO_FILE))
+        info = MFEnv.get_filename(MetaFile.INFO_FILE)


it would be good to do package.get_info_file_path() so that we don't have to bother decorator authors to worry about what is MFEnv or MetaFile or InfoFile

I didn't want to specialize too much I guess but I don't mind having separate utility functions get_info_file_path, get_dist_file_path etc if you prfer that. I was trying to make it easier to add files later.

savingoyal · 2025-05-29T21:17:31Z

metaflow/plugins/pypi/conda_decorator.py

@@ -159,11 +159,11 @@ def runtime_init(self, flow, graph, package, run_id):
            os.path.join(self.metaflow_dir.name, "metaflow"),


a future TODO: the path mangling here is something that the package module can handle by default. the same gymnastics are needed every single time for similar decorators.

savingoyal · 2025-05-29T21:19:11Z

metaflow/plugins/uv/bootstrap.py

@@ -93,6 +93,7 @@ def skip_metaflow_dependencies():
        return skip_pkgs

    def sync_uv_project(datastore_type):
+        # TODO: NEED TO POINT UV TO FILES IN THE CONFIG DIR


i believe this needs to be addressed before we merge this PR? cc @saikonen

savingoyal · 2025-05-29T21:19:58Z

metaflow/util.py

@@ -9,7 +9,6 @@
 from itertools import takewhile


this can be a different PR, cc @madhur-ob

I don't think that is a change I did. What was this supposed to do?

savingoyal · 2025-05-29T21:30:40Z

metaflow/meta_files.py

+MFENV_DIR = (
+    ".mfenv"  # Directory containing "system" code (metaflow and user dependencies)
+)
+MFCONF_DIR = ".mfconf"  # Directory containing Metaflow's configuration files


consider changing the name to something else since it is neither metaflow config not the new config feature

So we can have MFCODE_DIR and MFINFO_DIR ?

savingoyal · 2025-05-29T21:31:07Z

metaflow/meta_files.py

+    ".mfenv"  # Directory containing "system" code (metaflow and user dependencies)
+)
+MFCONF_DIR = ".mfconf"  # Directory containing Metaflow's configuration files
+MFENV_MARKER = (


can't this be just in the info file? will simplify things quite a bit

would make it less flexible. The marker tells you how to access the info file. The info file is not in the same place already.

savingoyal · 2025-05-29T21:33:26Z

metaflow/package/mfenv.py

+    CODE_MODULE = 0x502
+    CODE_METAFLOW = 0x504
+    CONFIG_FILE = 0x601
+    CONFIG_CONTENT = 0x202


why do we need to distinguish between all of them? wouldn't just user code, system code, info be sufficient minimally?

I'll have to check. We need to distinguish between files and content I think in some of the things. I can check if I can get rid of that. I think we also nee to know something is a module to extract the dist info.

savingoyal · 2025-05-29T21:36:23Z

metaflow/package/mfenv.py

+
+    @classmethod
+    def _extract_archive_mfenv_info(
+        cls, archive: Any, packaging_backend=TarPackagingBackend


any methods that need access to archive can be safely moved under MetaflowPackage?

I can check

savingoyal · 2025-05-29T21:38:23Z

metaflow/package/mfenv.py

+)
+
+
+class PackagedDistribution(metadata.Distribution):


consider moving this to a different file so that we can avoid an import from .mfnv in cli.py

savingoyal · 2025-05-29T21:43:52Z

metaflow/package/mfenv.py

+    _mappings = {}
+
+    @classmethod
+    def get_filename(


fair to assume that some methods work against local filesystem and some against the code package?

yes. I've tried to name them differently.

savingoyal · 2025-05-29T21:48:52Z

metaflow/meta_files.py

+    return None
+
+
+def read_included_dist_info():


consider renaming to read_dist_info()

romain-intel added 6 commits May 30, 2025 00:09

Make ConfigValue more dict-like

871faf9

Forgot to revert setting

c706438

New packaging support

e500118

madhur-ob force-pushed the only-packaging branch from 62ad8a9 to e500118 Compare May 29, 2025 18:47

madhur-ob added 3 commits May 30, 2025 00:19

fix stubs

1bc64a4

re-introduce config_decorators.py

01d2c16

fix 3_7 import again

47080d2

madhur-ob force-pushed the only-packaging branch from a3de2e6 to 47080d2 Compare May 29, 2025 19:23

encode only if str

0b47966

madhur-ob force-pushed the only-packaging branch from 7625d1e to 0b47966 Compare May 29, 2025 19:41

savingoyal reviewed May 29, 2025

View reviewed changes

romain-intel reviewed May 29, 2025

View reviewed changes

savingoyal reviewed May 29, 2025

View reviewed changes



		def generic_get_filename(
		name: Union[MetaFile, str], is_meta: Optional[bool] = None

		@@ -159,11 +159,11 @@ def runtime_init(self, flow, graph, package, run_id):
		os.path.join(self.metaflow_dir.name, "metaflow"),

packaging #2438

Are you sure you want to change the base?

packaging #2438

Uh oh!

Conversation

madhur-ob commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savingoyal May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

savingoyal May 29, 2025 •

edited

Loading