-
Notifications
You must be signed in to change notification settings - Fork 18
Add a CI check to ensure packages entries in pyproject.toml are up to date #29
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
Conversation
f753cce to
73677bf
Compare
0d33245 to
eddc10f
Compare
660ff58 to
ddc342c
Compare
| (tmp_path / "__init__.py").write_text("") | ||
|
|
||
| components_dir = tmp_path / "components" | ||
| components_dir.mkdir() | ||
| (components_dir / "__init__.py").write_text("") | ||
|
|
||
| training_dir = components_dir / "training" | ||
| training_dir.mkdir() | ||
| (training_dir / "__init__.py").write_text("") |
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.
This code is duplicated. It appears 3 times in this file.
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.
Removed the duplicate code and created common directory structure for the tests.
| if (repo_root / "__init__.py").exists(): | ||
| packages.add("kfp_components") | ||
|
|
||
| def _discover_recursive(directory: Path, base_package: str) -> None: |
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.
base_package is unused.
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.
Updated the function to use base_package.
| except FileNotFoundError: | ||
| raise RuntimeError(f"pyproject.toml not found at {pyproject_path}") | ||
| except tomllib.TOMLDecodeError as e: | ||
| raise RuntimeError(f"Failed to parse pyproject.toml: {e}") |
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.
The tomllib.TOMLDecodeError is caught and re-raised as a RuntimeError, which loses the original traceback context.
| raise RuntimeError(f"Failed to parse pyproject.toml: {e}") | |
| raise RuntimeError(f"Failed to parse pyproject.toml: {e}") from e |
| assert not is_valid | ||
| assert len(errors) == 1 | ||
| assert "Extra packages" in errors[0] | ||
| assert "kfp_components.components.nonexistent" in errors[0] |
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.
Don't we need to verify kfp_components.components too?
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.
Good catch! added an assert statement for kfp_components.components.
| import sys | ||
| import tomllib | ||
| from pathlib import Path | ||
| from typing import Set |
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.
The Set import from typing isn't needed in Python 3.9+. You can use builtin set[str] directly (like you already do with list[str] on lines 90 and 102). Consider updating lines 24, 30, and 66 to use set[str] and removing this import for consistency.
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.
Removed Set import and updated with set[str].
3f582eb to
1b515f5
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| sys.path.insert(0, str(Path(__file__).parent.parent)) | ||
|
|
||
| from validate_package_entries import ( | ||
| discover_packages, | ||
| read_pyproject_packages, | ||
| validate_package_entries, | ||
| ) |
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.
We're no longer using sys.path.insert(0, str(Path(__file__).parent.parent)). We're creating a __init.py__ file and using relative imports instead.
PTAL at https://github.com/kubeflow/pipelines-components/blob/main/scripts/README.md#import-conventions for more info.
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.
Updated to use relative imports.
| def get_repo_root() -> Path: | ||
| """Get the repository root directory.""" | ||
| return Path(__file__).parent.parent.parent.resolve() |
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.
I've sent a PR that includes this method as a lib for reuse.
#57
If that PR is merged before this one, please use that instead of duplicating it here.
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.
Updated to reuse get_repo_root() from common utility functions.
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.
We need to hold this PR until #57 gets merged.
7809865 to
1fdbbea
Compare
| if (repo_root / "__init__.py").exists(): | ||
| packages.add("kfp_components") | ||
|
|
||
| def _discover_recursive(directory: Path, base_package: str) -> None: |
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.
Optional:
Is this defined inside another function by purpose?
Personally, I think it's ugly. I understand though that it allows us to use packages without passing it as a parameter.
I'm fine with leaving it as it is, unless it was not intentional.
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.
Updated _discover_recursive as a separate function that takes packages as a parameter.
Signed-off-by: VaniHaripriya <[email protected]>
0da92b7 to
79cc441
Compare
hbelmiro
left a comment
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.
/lgtm
Description of your changes:
Added a new CI check to validate the package entries in pyproject.toml.
Sample run for an example of the CI working.
Checklist:
Pre-Submission Checklist
Additional Checklist Items for New or Updated Components/Pipelines:
metadata.yamlincludes freshlastVerifiedtimestampsnake_casenaming convention