-
Notifications
You must be signed in to change notification settings - Fork 25
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
Augment DockerRunner to support running from a fondant Pipeline #651
Conversation
b68a67c
to
1214da8
Compare
Thanks python 3.8... |
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.
Thanks @GeorgesLorre! The general approach looks good!
src/fondant/pipeline/runner.py
Outdated
extra_volumes=extra_volumes, | ||
build_args=build_args, | ||
) | ||
self.run(output_path) |
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.
Shouldn't this be _run
?
src/fondant/pipeline/runner.py
Outdated
self, | ||
input: t.Union[Pipeline, str], | ||
*, | ||
output_path: str = "docker-compose.yml", |
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.
Do we need to expose this? I would move this to a temp file or hidden directory.
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.
Wouldn't this make it more difficult to run the pipeline again from the same component spec to follow the "compile once, run many" approach (well in this case it would be "run once, run many")?
Unless this is temporary until we introduce a general compiler that we can start from as proposed here https://github.com/orgs/ml6team/projects/1/views/8?filterQuery=&pane=issue&itemId=44594106
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 would only hide this in the Runner. Users can still explicitly compile using the compile
command or Compiler
class.
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.
With expose you mean the file and not the argument ?
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.
Both
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
src/fondant/pipeline/runner.py
Outdated
self, | ||
input: t.Union[Pipeline, str], | ||
*, | ||
output_path: str = "docker-compose.yml", |
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 know super picky, but can we use a common yaml file extension within Fondant. Either yml
or yaml
?
btw: accordingly to the docker compose docs the new preferred default file for docker compose is called compose.yaml
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.
Fair point ( can we add this to precommit ?). I will make the changes
Are we planning on doing the same for kfp and Vertex? |
Yes the idea is to make the Runner classes prime user interfaces (next to the cli). Now you need to import a compiler and runner and and compile first before running. The idea here is that the user would only need 1 class that is easy to use. |
f745ac1
to
1c8c4d0
Compare
7c107d6
to
d2217d6
Compare
d2217d6
to
f5e2b72
Compare
No description provided.