Fix extensions and packaging for some types of editable package installs #2612
Conversation
There was a problem hiding this comment.
Pull Request Overview
Fixes extension discovery and packaging for editable installs by falling back to walking directories referenced by .pth files and editable finder modules, while keeping the non-editable case efficient.
- Traverse submodule search locations (including path hooks) to resolve extension roots for both distributions and PYTHONPATH packages.
- Track and use full_path_files for accurate initial packaging; add filtering that can leverage a custom filter_function from meta modules.
- Skip .pth and editable files when enumerating distribution files; adjust path handling and packaging logic accordingly.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| metaflow/packaging_sys/v1.py | Enhances module root path resolution and adjusts distribution file handling to accommodate editable installs. |
| metaflow/extension_support/init.py | Refactors extension discovery to walk editable paths, adds file filtering and packaging improvements, and introduces finder mapping logic. |
| README.md | Minor whitespace cleanup (non-functional). |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "Package '%s' defines more than one meta configuration: " | ||
| "'%s' and '%s' (at least)" | ||
| % ( | ||
| dist_name, | ||
| state["meta_module"], | ||
| potential_meta_module, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
dist_name is not defined in this scope inside process_file; this raises a NameError when multiple meta configs are detected. Use state['name'] (already present in state) instead.
Copilot uses AI. Check for mistakes.
| if filter_function: | ||
| new_files, new_full_path_files = [], [] | ||
| for short_file, full_file in zip(pkg["files"], pkg["full_path_files"]): | ||
| try: | ||
| if filter_function(os.path.join(EXT_PKG, short_file)): | ||
| new_files.append(short_file) | ||
| new_full_path_files.append(full_file) | ||
| except Exception as e: | ||
| _ext_debug( | ||
| " Exception '%s' when calling filter_function on " | ||
| "'%s', ignoring file" % (e, short_file) | ||
| ) | ||
| elif include_suffixes: | ||
| for short_file, full_file in zip(pkg["files"], pkg["full_path_files"]): | ||
| if any( | ||
| [short_file.endswith(suffix) for suffix in include_suffixes] | ||
| ): | ||
| new_files.append(short_file) | ||
| new_full_path_files.append(full_file) | ||
| elif exclude_suffixes: | ||
| for short_file, full_file in zip(pkg["files"], pkg["full_path_files"]): | ||
| if not any( | ||
| [short_file.endswith(suffix) for suffix in exclude_suffixes] | ||
| ): | ||
| new_files.append(short_file) | ||
| new_full_path_files.append(full_file) |
There was a problem hiding this comment.
new_files and new_full_path_files are used before assignment in the include_suffixes and exclude_suffixes branches, leading to UnboundLocalError. Initialize them before appending in those branches.
Copilot uses AI. Check for mistakes.
|
|
||
| # Temporary variables to support the loop below and make sure we loop through all | ||
| # the paths in the submodule_search_locations including calling the path hooks. | ||
| # We coudl skip calling things on the path hooks since the module was just imported |
There was a problem hiding this comment.
Typo in comment: 'coudl' should be 'could'.
| # We coudl skip calling things on the path hooks since the module was just imported | |
| # We could skip calling things on the path hooks since the module was just imported |
Copilot uses AI. Check for mistakes.
| new_paths.extend(addl_spec.submodule_search_locations) | ||
| # Remove .__path_hook__ and add .py to match the name of the file | ||
| # installed by the distribution | ||
| finder_name = p[:-14].translate(FINDER_TRANS) + ".py" |
There was a problem hiding this comment.
[nitpick] Avoid using the magic number 14 to strip the suffix; it's brittle. Prefer checking and removing a literal suffix, e.g., suffix = '.path_hook'; if p.endswith(suffix): finder_name = p[: -len(suffix)].translate(FINDER_TRANS) + '.py'.
Copilot uses AI. Check for mistakes.
| addl_spec is not None | ||
| and addl_spec.submodule_search_locations | ||
| ): | ||
| finder_name = p[:-14].translate(FINDER_TRANS) + ".py" |
There was a problem hiding this comment.
[nitpick] Same as above: replace the hard-coded slice with stripping a literal '.path_hook' suffix for clarity and robustness.
Copilot uses AI. Check for mistakes.
In some cases, a distribution install of an editable package includes just a .pth file and not the full file list included in the extension. In those cases, the extension mechanism failed to fall back to traversing the directory indicated in the .pth file. This PR fixes this issue while trying to maintain the "usual" case of non editable packages as quick as possible (walking the FS is not ideal in all cases). Some additional clean-up particularly around handling the case of "getting file from the distribution files list" and "walking a tree". We already used to walk the FS for non-distribution packages but now we do it in both cases (potentially) so the code was refactored to make it clearer. A similar issue could also happen when packaging METAFLOW_PACKAGE_POLICY packages that were installed in an editable mode. The same type of fixes are applied there as well.
romain-intel
force-pushed
the
fix/support_editable_pkgs_more
branch
from
f4aae3a to
cf3850b
Compare
| # contributes to. This is to enable multiple namespace packages to contribute | ||
| # to the same extension point (for example, you may have multiple packages | ||
| # that have plugins) | ||
| for f in dist.files: |
There was a problem hiding this comment.
For the case of a wheel that's been repacked by Bazel's rules_python, I believe it will still encounter an error on this line as dist.files will return None. Can we check for the None case and fall back to walking the file tree in that case?
| % _extension_points[idx] | ||
| ) | ||
| continue | ||
| for root, _, files in os.walk(package_path): |
There was a problem hiding this comment.
Consider whether we can set followlinks here. Bazel preps sandboxed directories using a lot of symlinks, so might get tripped up without this.
| for root, _, files in os.walk(package_path): | |
| for root, _, files in os.walk(package_path, followlinks=True): |
In some cases, a distribution install of an editable package includes just a .pth file and not the full file list included in the extension. In those cases, the extension mechanism failed to fall back to traversing the directory indicated in the .pth file.
This PR fixes this issue while trying to maintain the "usual" case of non editable packages as quick as possible (walking the FS is not ideal in all cases).
Some additional clean-up particularly around handling the case of "getting file from the distribution files list" and "walking a tree". We already used to walk the FS for non-distribution packages but now we do it in both cases (potentially) so the code was refactored to make it clearer.
A similar issue could also happen when packaging METAFLOW_PACKAGE_POLICY packages that were installed in an editable mode. The same type of fixes are applied there as well.