-
Notifications
You must be signed in to change notification settings - Fork 297
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
Imgspec/copy auto #2731
Imgspec/copy auto #2731
Conversation
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2731 +/- ##
===========================================
- Coverage 78.57% 49.75% -28.83%
===========================================
Files 193 194 +1
Lines 19672 19718 +46
Branches 4100 4109 +9
===========================================
- Hits 15458 9811 -5647
- Misses 3492 9379 +5887
+ Partials 722 528 -194 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
""" | ||
|
||
name: str = "flytekit" | ||
python_version: str = None # Use default python in the base image if None. | ||
builder: Optional[str] = None | ||
source_root: Optional[str] = None | ||
source_root: Optional[str] = None # a.txt:auto |
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.
a.txt:auto
. is there a plan to support this?
it looks nicer honestly
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.
separate pr?
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Why are the changes needed?
We introduced a new switch,
--copy
, to the pyflyte command recently to help standardize control of the fast register experience. This switch also changed the behavior slightly for discovering and copying the files into the fast register tar file. This PR changes the ImageSpec build process to also rely on that new file discovery and copying behavior, and updates the default behavior to copy only loaded Python modules rather than all files. This make all file finding/copying the same across flytekit.What changes were proposed in this pull request?
copy
to theImageSpec
object to mirror the option on the switches. This is the first time ImageSpec has a non-primitive option but I think this is okay.tools
andcore
side it was causing too many circular imports.Couple notes:
tag
andid
properties ofImageSpec
rely on the fieldssource_root
and nowcopy
to compute the tag and id correctly. This is fine but because these values are mutated by theupdate_image_spec_copy_handling
(this was happening before this pr also), this means that when a user calls these properties will affect their return values. We can makeupdate_image_spec_copy_handling
not mutate, but that doesn't work sincetag
/id
need the correct values forsource_root
andcopy
in order to correctly compute their values. Basically the question is how to make sure serialization settings has been taken into account before these two properties are called.deref_symlinks
and anyignore
settings are not piped all the way through. We should clean this up as a follow-up.How was this patch tested?
Unit tests, tested locally with sandbox, confirmed behavior before and after. Tested both
default
andenvd
builders. ls files inside image, ran a test workflow, etc.Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link