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

Add compile and run interface to kfp based runners #704

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

GeorgesLorre
Copy link
Collaborator

No description provided.

Comment on lines 122 to 124
self._run(output_path)
else:
self._run(input)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should pass the experiment_name here as well?

@@ -92,11 +97,37 @@ def _resolve_imports(self):
)

def run(
Copy link
Member

Choose a reason for hiding this comment

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

Can't help but notice that the flow of this method is the same for every runner. Might be worth it to lift it to the base Runner class. Up to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the same idea when commuting home, will also make testing a lot easier

@@ -121,7 +121,7 @@ def run(
)
self._run(output_path)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be added here as well? :)

@GeorgesLorre GeorgesLorre force-pushed the feature/align-kfp-based-runners branch from 54ba090 to e4103c6 Compare December 7, 2023 10:30
Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks @GeorgesLorre!

@GeorgesLorre GeorgesLorre merged commit 1f07824 into main Dec 7, 2023
6 checks passed
@GeorgesLorre GeorgesLorre deleted the feature/align-kfp-based-runners branch December 7, 2023 14:26
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.

2 participants