-
Notifications
You must be signed in to change notification settings - Fork 14
Add compile check tooling and CI workflow #13
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: main
Are you sure you want to change the base?
Conversation
3ae7fef to
fe439e8
Compare
|
/ok-to-test |
4f3a624 to
0abdee9
Compare
68cbff0 to
31c36f0
Compare
| @@ -0,0 +1,382 @@ | |||
| #!/usr/bin/env python3 | |||
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.
Could you move this script and its test to .github/scripts by following this- https://github.com/kubeflow/pipelines-components/blob/main/.github/scripts/README.md
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.
Sure, moved it to .github/scripts
| run: | | ||
| python scripts/compile_check.py --tier all | ||
|
|
||
| - name: Run unit tests |
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.
Once you move the compile_check.py and test_compile_check.py to .github/scripts, you can remove this step, since https://github.com/kubeflow/pipelines-components/blob/main/.github/workflows/scripts-tests.yml takes care of running the tests under .github/scripts
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.
Done, moved both scripts to .github/scripts and removed the step mentioned
ba47cce to
57fedc5
Compare
| from pathlib import Path | ||
| from typing import Dict, Iterable, List, Optional, Sequence, Tuple | ||
|
|
||
| try: |
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 try-except block can be removed, since pyyaml is a required dependency listed in pyproject.toml.
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 block and pushed updated changes
17b3c4e to
0950e84
Compare
|
/retest |
1e1873c to
8633626
Compare
Signed-off-by: sduvvuri1603 <[email protected]>
Signed-off-by: sduvvuri1603 <[email protected]>
228f419 to
f5080c8
Compare
| on: | ||
| pull_request: | ||
| branches: | ||
| - main |
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.
could you add the required path filters to trigger this workflow-
paths:
- 'components/'
- 'pipelines/'
- '.github/scripts/compile_check/'
- '.github/workflows/compile-and-deps.yml'
- '.github/actions/setup-python-ci/'
- 'pyproject.toml'
- 'uv.lock'
| """Parse command-line arguments for the compile check tool.""" | ||
| parser = argparse.ArgumentParser(description="Compile Kubeflow components and pipelines.") | ||
| parser.add_argument( | ||
| "--tier", |
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 think we can remove the tier filtering since we are not going to have third party components or pipelines.
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 tier filtering accordingly
99c0e3e to
0a5fdb1
Compare
Signed-off-by: sduvvuri1603 <[email protected]>
cc6afeb to
96403fe
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 |
| ) | ||
| return component_dir | ||
|
|
||
| def test_successful_compile(self) -> 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.
Could add a test for pipeline compilation, invalid component and pipeline.
|
@sduvvuri1603 Could you create a sample run for the new workflow. |
| import traceback | ||
| from dataclasses import dataclass, field | ||
| from pathlib import Path | ||
| from typing import Dict, Iterable, List, Optional, Sequence, Tuple |
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.
To keep it consistent wherever it is applicable,please update imports to use built-in generic types for Python 3.9+: Dict → dict, List → list, Tuple → tuple .
Summary
add scripts/compile_check.py to discover metadata-based components/pipelines, validate dependency specs, and compile each target with the KFP SDK
add scripts/test_compile_check.py(Unit Tests) to exercise a passing component and an invalid dependency case in a temporary repo layout
wire .github/workflows/compile-and-deps.yml so every PR runs the compile checker and its unit tests on Python 3.10 using uv
Testing
scripts/compile_check.py --tier all --verbose) to confirm metadata discovery and compilation logic succeeds after the PyYAML install fix—no targets yet, so it exits cleanly.python -m pytest) in a virtualenv; both scenarios inscripts/test_compile_check.pypass (valid component compiles, malformed dependency is rejected). This also proves the__init__.pyimport fallback works when pytest imports the package directly.