APPENG-2959: Add python transitive support on top of NAT framework#100
Merged
Conversation
zvigrinberg
requested changes
Sep 2, 2025
zvigrinberg
left a comment
zvigrinberg
left a comment
Collaborator
There was a problem hiding this comment.
Hi @TamarW0
Good job!.
Please see my comments, some of them requires changes.
Thanks!.
zvigrinberg
reviewed
Sep 4, 2025
zvigrinberg
reviewed
Sep 4, 2025
zvigrinberg
requested changes
Sep 4, 2025
zvigrinberg
left a comment
zvigrinberg
left a comment
Collaborator
There was a problem hiding this comment.
Hi @TamarW0,
I Have few minor comments, please take a look.
Thanks.
Comment on lines
+342
to
+514
|
|
||
|
|
||
| class PythonDependencyTreeBuilder(DependencyTreeBuilder): | ||
|
|
||
| def build_tree(self, manifest_path: Path) -> defaultdict[Any, list]: | ||
| cmd = f'{manifest_path}/{TRANSITIVE_ENV_NAME}/bin/python -m pip install deptree' | ||
| run_command(cmd) | ||
| cmd = f'{manifest_path}/{TRANSITIVE_ENV_NAME}/bin/deptree', | ||
| dependencies = run_command(cmd) | ||
| parent_stack = [] | ||
| tree = defaultdict(set) | ||
| ROOT_PROJECT = 'root_project' | ||
| tree[ROOT_PROJECT] = [ROOT_LEVEL_SENTINEL] | ||
| for line in dependencies.split(os.linesep): | ||
| level = 0 | ||
| while line.startswith(' '): | ||
| level += 1 | ||
| line = line[2:] | ||
| package = line.split('==')[0].strip().lower() | ||
| package = package.replace('-', '_') | ||
| if level < len(parent_stack): | ||
| parent_stack = parent_stack[:level] | ||
| try: | ||
| tree[package].add(parent_stack[-1]) | ||
| except IndexError: | ||
| pass | ||
| parent_stack.append(package) | ||
|
|
||
| installed_dependencies = [] | ||
| with open(manifest_path / PYTHON_MANIFEST, 'r') as manifest: | ||
| for line in manifest: | ||
| if line.strip() and not PythonLanguageFunctionsParser.is_comment_line(line): | ||
| installed_dependencies.append(re.split(r"[=>< ]", line.strip())[0]) | ||
| for dependency, parents in tree.items(): | ||
| if dependency in installed_dependencies: | ||
| parents.add(ROOT_PROJECT) | ||
| tree[dependency] = list(parents) | ||
| return tree | ||
|
|
||
| def extract_version_from_specifier(self, specifier_str: str) -> Optional[str]: | ||
| """ | ||
| Extracts a most likely specific Python version from a PEP 440 specifier string. | ||
| Prioritizes exact matches, then lower bounds. | ||
| Examples: | ||
| "==3.9" -> "3.9" | ||
| ">=3.8,<4.0" -> "3.8" | ||
| "~=3.7" -> "3.7" (if ~=3.7 is equivalent to >=3.7,<3.8) | ||
| ">3.8" -> "3.9" (returns the next major.minor version) | ||
| """ | ||
| try: | ||
| specifier_set = SpecifierSet(specifier_str) | ||
|
|
||
| for specifier in specifier_set: | ||
| if specifier.operator in ('==', '==='): | ||
| return specifier.version | ||
|
|
||
| lower_bounds = [] | ||
| for specifier in specifier_set: | ||
| if specifier.operator in ('>=',): | ||
| lower_bounds.append(self.parse_version(specifier.version)) | ||
| if lower_bounds: | ||
| # Return the highest (most restrictive) lower bound | ||
| highest_lower_bound = max(lower_bounds) | ||
| return str(highest_lower_bound) | ||
|
|
||
| # Look for a compatible release specifier (~=3.7 implies >=3.7,<3.8) | ||
| compatible_match = re.search(r'~=(\d+\.\d+)', specifier_str) | ||
| if compatible_match: | ||
| return compatible_match.group(1) | ||
|
|
||
| # As a fallback for ">X.Y" which means X.Y+1.0, return the next minor version | ||
| greater_than_match = re.search(r'>(\d+\.\d+)', specifier_str) | ||
| if greater_than_match: | ||
| major, minor = map(int, greater_than_match.group(1).split('.')) | ||
| return f"{major}.{minor + 1}" | ||
|
|
||
| except Exception as e: # Catch parsing errors from packaging.specifiers | ||
| logger.warning(f"Warning: Could not parse specifier '{specifier_str}': {e}") | ||
| return None | ||
|
|
||
| def extract_version_from_readme_hint(self, line: str) -> Optional[str]: | ||
| """ | ||
| Extracts a Python version (e.g., 3.8, 3.12) from a free-form text line. | ||
| Looks for patterns like 'Python 3.9', 'python3.10', 'py3.11'. | ||
| """ | ||
| match = re.search(r'(?:Python|python|py)\s*(\d+\.\d+)', line, re.IGNORECASE) | ||
| return match.group(1) if match else None | ||
|
|
||
| def extract_version_from_pyproject_toml(self, content: str) -> Optional[str]: | ||
| #todo: implement more options here, see example: https://github.com/quay/quay/blob/master/pyproject.toml | ||
| try: | ||
| pyproject_data = json.loads(content) | ||
| specifier_from_metadata = pyproject_data.get('project', {}).get('requires-python', None) | ||
| if specifier_from_metadata: | ||
| logger.debug(f"Found requires-python in pyproject.toml: '{specifier_from_metadata}'") | ||
| # Try to extract a specific version from the specifier | ||
| specific_version = self.extract_version_from_specifier(specifier_from_metadata) | ||
| if specific_version: | ||
| return specific_version # Return the most authoritative version immediately | ||
| return '' | ||
| except json.JSONDecodeError: | ||
| logger.warning(f"Warning: Failed to parse pyproject.toml as JSON.") | ||
|
|
||
| def extract_version_from_setup_py(self, content: str): | ||
| try: | ||
| tree = ast.parse(content) | ||
| for node in ast.walk(tree): | ||
| if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == 'setup': | ||
| for keyword in node.keywords: | ||
| if keyword.arg == 'python_requires': | ||
| if isinstance(keyword.value, ast.Constant): | ||
| specifier_from_metadata = keyword.value.value | ||
| logger.debug(f"Found python_requires in setup.py: '{specifier_from_metadata}'") | ||
| specific_version = self.extract_version_from_specifier(specifier_from_metadata) | ||
| if specific_version: | ||
| return specific_version | ||
| break | ||
| except SyntaxError: | ||
| logger.warning(f"Warning: Failed to parse setup.py as Python code") | ||
| return '' | ||
|
|
||
| def extract_version_from_readme_md(self, content: str): | ||
| version_from_readme_hint = '' | ||
| for line in content.splitlines(): | ||
| extracted_version = self.extract_version_from_readme_hint(line) | ||
| if extracted_version: | ||
| if not version_from_readme_hint or ( | ||
| self.extract_version_from_specifier(extracted_version) > self.extract_version_from_specifier( | ||
| version_from_readme_hint) | ||
| ): | ||
| version_from_readme_hint = extracted_version | ||
| logger.debug(f"Found Python version hint in README.md: '{extracted_version}'") | ||
| return version_from_readme_hint | ||
|
|
||
| def determine_python_version(self, git_repo_path: str) -> Optional[str]: | ||
| """ | ||
| Determines the most specific Python version from project information documents. | ||
| Prioritizes pyproject.toml, then setup.py, then README.md. | ||
| """ | ||
|
|
||
| info_docs_mapping = {PYPROJECT_TOML: self.extract_version_from_pyproject_toml, | ||
| SETUP_PY: self.extract_version_from_setup_py, | ||
| README_MD: self.extract_version_from_readme_md} | ||
|
|
||
| for doc, logic in info_docs_mapping.items(): | ||
| for root, _, files in os.walk(git_repo_path): | ||
| if doc in files: | ||
| doc_full_path = os.path.join(root, doc) | ||
| with open(doc_full_path, 'r') as file: | ||
| python_version = logic(file.read()) | ||
| if python_version: | ||
| return python_version | ||
| return '3.11' | ||
|
|
||
| def install_dependencies(self, git_repo_path): | ||
| cmd = f'cd {git_repo_path} && python -m venv {TRANSITIVE_ENV_NAME}' | ||
| run_command(cmd) | ||
| with open(git_repo_path / PYTHON_MANIFEST, 'r') as manifest: | ||
| for line in tqdm(manifest): | ||
| if line.strip() and not PythonLanguageFunctionsParser.is_comment_line(line): | ||
| self.install_dependency(line, git_repo_path) | ||
|
|
||
| def install_dependency(self, dependency, repo_path): | ||
| valid_signs = ['==', '>=', '<=', '!='] | ||
| if not any([sign in dependency for sign in valid_signs]): | ||
| dependency = dependency.replace('=', '==') | ||
| cmd = f'{repo_path}/{TRANSITIVE_ENV_NAME}/bin/python -m pip install {dependency}' | ||
| res = run_command(cmd) | ||
| if not res: | ||
| logger.warning(f'Failed to install dependency {dependency}') | ||
|
|
||
|
|
||
|
|
Collaborator
There was a problem hiding this comment.
@TamarW0 You have a duplication of the class PythonDependencyTreeBuilder
Please remove one of them ( make sure that they're identical, if not, keep the one that is the correct one).
Collaborator
There was a problem hiding this comment.
@TamarW0 I Still see duplication of PythonDependencyTreeBuilder
This was referenced Sep 7, 2025
zvigrinberg
approved these changes
Sep 7, 2025
zvigrinberg
left a comment
zvigrinberg
left a comment
Collaborator
There was a problem hiding this comment.
@TamarW0 LGTM Approved.
just before Merging, please try to deploy the temp tag on cluster ( with self hosted variant) and verify it works as expected also on cluster.
Also kindly squash all commits into one.
Thank you for your efforts!.
Collaborator
Author
|
/retest |
TamarW0
force-pushed
the
APPENG-2959-migration
branch
from
bd36466 to
2c170a2
Compare
TamarW0
force-pushed
the
APPENG-2959-migration
branch
from
1eb8b13 to
72e9859
Compare
Collaborator
Author
|
/retest |
heatherzh01
pushed a commit
that referenced
this pull request
Mar 6, 2026
APPENG-2959: Add python transitive support on top of NAT framework
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.