Skip to content
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

Minor change to kfp workflow. #684

Merged
merged 8 commits into from
Oct 10, 2024
Merged

Minor change to kfp workflow. #684

merged 8 commits into from
Oct 10, 2024

Conversation

revit13
Copy link
Collaborator

@revit13 revit13 commented Oct 8, 2024

Following request from @roytman and @daw3rd this pr tries to merge common parts in kfp workflow different steps.
In addition this PR builds the workflow even if its part of the workflow's black list.

@revit13 revit13 force-pushed the kfp-workflow1 branch 2 times, most recently from d568e3d to 2490495 Compare October 9, 2024 10:12
@revit13 revit13 requested review from daw3rd and roytman October 9, 2024 10:14
Copy link
Member

@roytman roytman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

echo "Skipping transforms/code/code_quality kfp test for lack of Makefile and/or kfp_ray/Makefile"
transform=$(basename "transforms/code/code_quality")
if echo ${KFP_BLACK_LIST} | grep -qv ${transform}; then
$PWD/scripts/workflow_helper.sh install-tools
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have makefile targets that call these? This would then allow the developer to run these tests locally, more easily and through mechanisms they are already familiar with (i.e. make).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also force all the kfp tests to be run - they are not in the current PR . See that only 3 checks were run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be okay to address this in a separate issue? Personally, I'm not in favor of this approach because this install-tools function works specifically for Ubuntu, as used in the GitHub Action. The installation process differs for other operating systems, like macOS, with its various versions. I believe it would be simpler for each person to deploy or use their own tools on their machine, rather than creating a new script that we would need to maintain. However, I could be wrong on this...

Signed-off-by: Revital Sur <[email protected]>
@daw3rd daw3rd merged commit 4c7f4a5 into IBM:dev Oct 10, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants