-
Notifications
You must be signed in to change notification settings - Fork 30
Support for build step-enabled pipelines #213
Conversation
…y into `create_python_pipeline`
Tests are currently failing due to a bug in 1.11.0, but I'll be able to fix it as soon as 1.11.2 is out. |
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 did have few questions questions for you (I think they'd really help us as maintainers), but I want to make sure you're able to merge this before you leave and none of them are like critical blockers, so LGTM.
raise Exception("build path {} does not exist".format(build_path)) | ||
if (build_path / ".pachignore").exists(): | ||
warnings.warn( | ||
"detected a '.pachignore' file, but it's unsupported by python_pachyderm -- use `pachctl` instead", |
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.
For posterity: #221
|
||
image = transform.build.image | ||
if not image: | ||
version = self.get_remote_version() |
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.
Is the idea that the standard build pipeline images follow pachd's versions? If so we'll need to make it part of our release process to update the image tags
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.
Yes that's correct. Builder images are already automatically tagged and pushed as part of the release process.
) | ||
) | ||
|
||
self.create_repo(build_pipeline_name, update=True) |
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'm probably just blind but is the ***_source
repo created? Does that need to happen?
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.
Also, this is just a general question about the feature, but why do we need to create the _build
repo separately (rather than having create_pipeline
do it)? Is it like a consistency thing?
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 build pipelines (but unlike the version of
create_python_pipeline
that this is replacing), there is only the_build
repo, which contains the source code in one branch, and the build assets pipeline outputting to another branch. - I'm not sure if I follow your second question. Is it "why are we calling
self._req
directly rather thanself.create_pipeline
in the following line?"
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 I just didn't quite understand how this feature worked (that the source and build output were two branches in the same repo), but this clarifies it—thanks!
src/python_pachyderm/mixin/pps.py
Outdated
return i.pfs.name | ||
if i.cross is not None: | ||
if len(i.cross) > 0: | ||
return pipeline_input_name(i.cross[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.
Just to check that I understand how this code works, technically you could return the empty string here, right? Like, IIUC, the idea is that you're assembling an iterator over all inputs (pipeline_inputs
below) and then converting to a list of pipeline names, but only the names of the PFS inputs matter because that's all you check for above?
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.
Yeah that's right, I'll fix this. This current function is just a transliteration of the equivalent code in go (I think in ppsutil
.)
and backported functionality into
create_python_pipeline
.The bulk of new functionality (i.e. in
Client.create_pipeline
) is basically a transliteration of the equivalent go code here.Closes #190