-
Notifications
You must be signed in to change notification settings - Fork 15
Add pip-package script-content discovery (Phase 1 prototype) #274
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
base: migrate-automation-engine-from-mlperf-automations
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
| import logging | ||
| import re | ||
| import shutil | ||
| import hashlib | ||
| import importlib.metadata as importlib_metadata | ||
| from pathlib import Path | ||
|
|
||
| from .logger import logger, setup_logging | ||
|
|
@@ -15,6 +17,147 @@ | |
| from .item import Item | ||
| from .error_codes import WarningCode | ||
|
|
||
| # Entry-point group that pip-installable script-content packages advertise | ||
| # themselves under (e.g. a package shipping the mlperf-automations script/ | ||
| # tree, or a fork of it under a different distribution name). | ||
| SCRIPT_PACKAGE_ENTRY_POINT_GROUP = "mlc.script_packages" | ||
|
|
||
|
|
||
| def _get_script_package_entry_points(): | ||
| """ | ||
| Version-compatible retrieval of entry points in | ||
| SCRIPT_PACKAGE_ENTRY_POINT_GROUP. Python 3.10+ supports | ||
| entry_points(group=...) directly; 3.8/3.9 only support the argument-less | ||
| entry_points(), which returns a plain dict (or dict-like | ||
| SelectableGroups) keyed by group name. | ||
| """ | ||
| try: | ||
| return list(importlib_metadata.entry_points( | ||
| group=SCRIPT_PACKAGE_ENTRY_POINT_GROUP)) | ||
| except TypeError: | ||
| # Python < 3.10: entry_points() takes no 'group' kwarg. | ||
| try: | ||
| all_eps = importlib_metadata.entry_points() | ||
| return list(all_eps.get(SCRIPT_PACKAGE_ENTRY_POINT_GROUP, [])) | ||
| except Exception as e: | ||
| logger.debug( | ||
| f"Could not enumerate '{SCRIPT_PACKAGE_ENTRY_POINT_GROUP}' " | ||
| f"entry points on this Python version: {e}") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback for Python < 3.10 entry_points API is well-handled, but consider logging at debug level only to avoid noisy logs in normal runs. |
||
| return [] | ||
| except Exception as e: | ||
| logger.debug( | ||
| f"Could not enumerate '{SCRIPT_PACKAGE_ENTRY_POINT_GROUP}' entry points: {e}") | ||
| return [] | ||
|
|
||
|
|
||
| def _pip_repo_cache_path(repos_path): | ||
| return os.path.join(repos_path, "package_repos_cache.json") | ||
|
|
||
|
|
||
| def _load_pip_repo_cache(repos_path): | ||
| cache_file = _pip_repo_cache_path(repos_path) | ||
| if not os.path.isfile(cache_file): | ||
| return {} | ||
| try: | ||
| with open(cache_file, "r") as f: | ||
| data = json.load(f) | ||
| return data if isinstance(data, dict) else {} | ||
| except (json.JSONDecodeError, OSError): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cache file path uses os.path.join but os is not imported; add 'import os' at the top. |
||
| return {} | ||
|
|
||
|
|
||
| def _save_pip_repo_cache(repos_path, cache): | ||
| try: | ||
| os.makedirs(repos_path, exist_ok=True) | ||
| with open(_pip_repo_cache_path(repos_path), "w") as f: | ||
| json.dump(cache, f, indent=2) | ||
| except OSError as e: | ||
| logger.debug(f"Could not save pip script package cache: {e}") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Loading the cache silently returns empty dict on JSON errors or OSErrors; consider logging a debug message to aid troubleshooting. |
||
|
|
||
|
|
||
| def discover_pip_script_repos(repos_path): | ||
| """ | ||
| Discover script-content repos advertised by installed pip packages via | ||
| the SCRIPT_PACKAGE_ENTRY_POINT_GROUP entry-point group. Returns a list | ||
| of Repo objects, one per distinct installed *distribution*. | ||
|
|
||
| Deliberately aliases each repo by the entry point's owning distribution | ||
| name (`ep.dist.name`), NOT the self-chosen entry-point name (`ep.name`). | ||
| PyPI enforces distribution-name uniqueness the same way GitHub enforces | ||
| owner/repo uniqueness, so this is what lets a fork of a script-content | ||
| package coexist with the original even if both declare the same | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Saving the cache swallows OSError with debug log only; this is reasonable but consider warning if saving fails repeatedly. |
||
| entry-point name and even if the fork copied script uids verbatim. | ||
|
|
||
| entry_points() itself is a fast, local, no-network scan of installed | ||
| *.dist-info/entry_points.txt files (the same mechanism `pip list` relies | ||
| on) and is called every run - it's cheap. What's NOT free is `.load()`, | ||
| which actually imports the resolver module; that's cached across runs in | ||
| <repos_path>/package_repos_cache.json keyed by distribution name, so an | ||
| already-known package's content dir is reused directly without | ||
| re-importing anything. | ||
|
|
||
| Distributions that disappear (uninstalled) are simply left out of the | ||
| returned list - Index.build_index()'s existing deleted-item detection | ||
| (comparing each run's indexed files against modified_times.json) already | ||
| prunes their content automatically, so no explicit | ||
| Index.remove_repo_from_index() call is needed here. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In discover_pip_script_repos, the cache key is distribution name, which is good for uniqueness. However, if two distributions share the same name but different versions, this might cause issues; consider versioning or hashing content_dir. |
||
| """ | ||
| cache = _load_pip_repo_cache(repos_path) | ||
| updated_cache = {} | ||
| discovered = {} | ||
| cache_changed = False | ||
|
|
||
| for ep in _get_script_package_entry_points(): | ||
| dist = getattr(ep, 'dist', None) | ||
| dist_name = dist.name if dist is not None else ep.name | ||
| if dist_name in discovered: | ||
| # Same distribution declaring more than one entry point in this | ||
| # group - first one found wins, it's still the same repo. | ||
| continue | ||
|
|
||
| content_dir = cache.get(dist_name) | ||
| if content_dir and os.path.isdir(content_dir): | ||
| # Already known from a previous run - reuse it, skip the | ||
| # (import-triggering) ep.load() call entirely. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of hashlib.md5 for uid generation is acceptable here, but document that this is not for security purposes, just stable unique IDs. |
||
| updated_cache[dist_name] = content_dir | ||
| else: | ||
| try: | ||
| module = ep.load() | ||
| content_dir = os.path.dirname(os.path.abspath(module.__file__)) | ||
| except Exception as e: | ||
| logger.warning( | ||
| f"Skipping script package entry point '{ep.name}' from " | ||
| f"distribution '{dist_name}': failed to load ({e})") | ||
| continue | ||
|
|
||
| if not os.path.isdir(content_dir): | ||
| logger.warning( | ||
| f"Script package '{dist_name}' entry point resolved to " | ||
| f"a non-existent directory: {content_dir}. Skipping.") | ||
| continue | ||
|
|
||
| updated_cache[dist_name] = content_dir | ||
| cache_changed = True | ||
| logger.info( | ||
| f"Discovered new script package '{dist_name}' at {content_dir}") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cache is saved only if changed, which is efficient. Consider adding a test to verify cache invalidation when a package is uninstalled. |
||
|
|
||
| uid = hashlib.md5(dist_name.encode('utf-8')).hexdigest()[:16] | ||
| discovered[dist_name] = Repo(path=updated_cache[dist_name], meta={ | ||
| 'alias': dist_name, | ||
| 'uid': uid, | ||
| 'source': 'pip', | ||
| }) | ||
|
|
||
| if set(cache.keys()) - set(updated_cache.keys()): | ||
| # one or more previously-known packages disappeared (uninstalled) | ||
| cache_changed = True | ||
|
|
||
| if cache_changed: | ||
| _save_pip_repo_cache(repos_path, updated_cache) | ||
|
|
||
| return list(discovered.values()) | ||
|
|
||
|
|
||
| # Base class for actions | ||
|
|
||
|
|
||
|
|
@@ -27,6 +170,18 @@ class Action: | |
| current_repo_path = None | ||
| repos = [] # list of Repo objects | ||
|
|
||
| @staticmethod | ||
| def _is_pip_sourced_repo(repo_obj): | ||
| """ | ||
| True if repo_obj (a Repo instance) was discovered from an installed | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The _is_pip_sourced_repo static method relies on repo.meta['source']=='pip'; ensure all pip repos have this meta set consistently. |
||
| pip package rather than a git-cloned/registered repo. Its files live | ||
| inside the Python environment's site-packages, not ~/MLC/repos, and | ||
| must not be mutated by mlc add/cp/mv/rm - that would corrupt the | ||
| installed package outside of pip's own bookkeeping. | ||
| """ | ||
| return bool(repo_obj) and getattr( | ||
| repo_obj, 'meta', {}).get('source') == 'pip' | ||
|
|
||
| # Main access function to simulate a Python interface for CLI | ||
| def access(self, options): | ||
| """ | ||
|
|
@@ -248,7 +403,12 @@ def __init__(self): | |
| if not os.path.exists(self.local_cache_path): | ||
| os.makedirs(self.local_cache_path, exist_ok=True) | ||
|
|
||
| self.repos = self.load_repos_and_meta() | ||
| # Pip-sourced script-content repos are prepended (processed first), | ||
| # so git-cloned repos loaded below are processed last and win any | ||
| # script-uid conflicts - pip is a second, lower-trust source for now | ||
| # (see mlcflow-pip-script-packages-proposal.pptx, Phase 1). | ||
| self.repos = discover_pip_script_repos( | ||
| self.repos_path) + self.load_repos_and_meta() | ||
| # logger.info(f"In Action class: {self.repos_path}") | ||
| self._index = None | ||
|
|
||
|
|
@@ -306,6 +466,10 @@ def add(self, i): | |
|
|
||
| # Determine paths and metadata format | ||
| repo = res["list"][0] | ||
| if self._is_pip_sourced_repo(repo): | ||
| return { | ||
| 'return': 1, | ||
| 'error': f"""Repo '{repo.meta.get('alias')}' is sourced from an installed pip package (read-only) - cannot add items to it. Use a git-cloned repo or the local repo instead."""} | ||
| repo_path = repo.path | ||
|
|
||
| target_name = i.get('target_name', self.action_type) | ||
|
|
@@ -425,11 +589,18 @@ def rm(self, i): | |
| force_remove = True | ||
|
|
||
| results = res['list'] | ||
| removed_paths = [] | ||
|
|
||
| for result in results: | ||
| item_path = result.path | ||
| item_meta = result.meta | ||
|
|
||
| if self._is_pip_sourced_repo(getattr(result, 'repo', None)): | ||
| logger.warning( | ||
| f"Skipping removal of {item_path}: sourced from an " | ||
| f"installed pip package (read-only), not a git-cloned repo.") | ||
| continue | ||
|
|
||
| if os.path.exists(item_path): | ||
| if force_remove == True: | ||
| shutil.rmtree(item_path) | ||
|
|
@@ -445,10 +616,21 @@ def rm(self, i): | |
| f"{target_name} item: {item_path} has been successfully removed") | ||
|
|
||
| self.get_index().rm(item_meta, target_name, item_path) | ||
| removed_paths.append(item_path) | ||
|
|
||
| if not removed_paths: | ||
| return { | ||
| "return": 0, | ||
| "warnings": [{"code": WarningCode.EMPTY_TARGET.code, | ||
| "description": f"No {target_name} item was actually removed " | ||
| f"(skipped: read-only pip-sourced repo, or declined confirmation)"}], | ||
| "message": "No items were removed", | ||
| } | ||
|
|
||
| return { | ||
| "return": 0, | ||
| "message": f"Item {item_path} successfully removed", | ||
| "message": f"Item {removed_paths[-1]} successfully removed", | ||
| "removed": removed_paths, | ||
| } | ||
|
|
||
| def save_new_meta(self, i, item_id, item_name, | ||
|
|
@@ -642,20 +824,37 @@ def cp(self, run_args): | |
| return { | ||
| 'return': 1, 'error': f"""Current directory is not inside a registered MLC repo and so using ".:" is not valid"""} | ||
| target_repo_name = os.path.basename(self.current_repo_path) | ||
| else: | ||
| if not any(os.path.basename(repodata.path) == | ||
| target_repo_name for repodata in self.repos): | ||
| return {'return': 1, 'error': f"""The target repo {target_repo} is not registered in MLC. Either register in MLC by cloning from Git through command `mlc pull repo` or create repo using `mlc add repo` command and try to rerun the command again"""} | ||
| target_repo_path = os.path.join(self.repos_path, target_repo_name) | ||
|
|
||
| # Match by alias first, across ALL repos, before ever falling | ||
| # back to basename - the on-disk directory basename doesn't | ||
| # always match the repo's alias (e.g. a pip-sourced repo's | ||
| # site-packages directory "mlc_scripts_content" vs its | ||
| # distribution-name alias "mlc-scripts"). Checking both in a | ||
| # single predicate would let an unrelated repo's basename match | ||
| # shadow the correct alias match depending on list order; doing | ||
| # alias as its own full pass first avoids that ambiguity. | ||
| target_repo = next( | ||
| (k for k in self.repos if os.path.basename( | ||
| k.path) == target_repo_name), None) | ||
| (k for k in self.repos if k.meta.get('alias') == target_repo_name), | ||
| None) | ||
| if target_repo is None: | ||
| target_repo = next( | ||
| (k for k in self.repos | ||
| if os.path.basename(k.path) == target_repo_name), None) | ||
|
|
||
| if target_repo is None: | ||
| return {'return': 1, 'error': f"""The target repo {target_repo_name} is not registered in MLC. Either register in MLC by cloning from Git through command `mlc pull repo` or create repo using `mlc add repo` command and try to rerun the command again"""} | ||
| target_repo_path = target_repo.path | ||
| target_item_name = target_split[1].strip() | ||
| else: | ||
| target_repo = result.repo | ||
| target_repo_path = result.repo.path | ||
| target_item_name = target_split[0].strip() | ||
|
|
||
| if self._is_pip_sourced_repo(target_repo): | ||
| return { | ||
| 'return': 1, | ||
| 'error': f"""Target repo '{getattr(target_repo, 'meta', {}).get('alias')}' is sourced from an installed pip package (read-only) - cannot copy/move items into it."""} | ||
|
|
||
| target_item_path = os.path.join( | ||
| target_repo_path, action_target, target_item_name) | ||
| res = self.copy_item(src_item_path, target_item_path) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -709,6 +709,11 @@ def rm(self, run_args): | |
| repo = list_repos[0] | ||
| repo_path = repo.path | ||
|
|
||
| if Action._is_pip_sourced_repo(repo): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error message for attempting to remove a pip-sourced repo is clear and instructive; consider adding a test to cover this case. |
||
| return { | ||
| 'return': 1, | ||
| 'error': f"""Repo '{repo.meta.get('alias')}' is sourced from an installed pip package, not a git clone - there is nothing to remove from ~/MLC/repos for it, and its files must not be deleted directly (that would corrupt the pip installation outside of pip's own bookkeeping). Run `pip uninstall {repo.meta.get('alias')}` instead; it will then stop being discovered automatically."""} | ||
|
|
||
| else: | ||
| repo = run_args['repo'] | ||
| if os.path.exists(repo): | ||
|
|
||
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.
Consider adding a module-level docstring summarizing the new pip script package discovery feature and its usage.