-
Notifications
You must be signed in to change notification settings - Fork 242
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
MLCOMPUTE-2733 | Add paasta names to spark driver pod annotations #4010
base: master
Are you sure you want to change the base?
Conversation
b274bf0
to
aa659ec
Compare
268ec57
to
fc2080f
Compare
paasta_tools/tron_tools.py
Outdated
extra_volumes=spark_tools.docker_volumes_to_mappings( | ||
self.get_volumes( | ||
system_paasta_config.get_volumes(), | ||
uses_bulkdata_default=system_paasta_config.get_uses_bulkdata_default(), | ||
), |
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.
was this change meant to be included in this diff?
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 it's because we updated service_configuration_lib (Yelp/service_configuration_lib#152) and the change is for making the typing matches. (didn't use direct cast since it's more risky)
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.
ah, gotcha!
i think that casting might actually be fine here (since we're going from a pretty concrete TypedDict to a pretty abstract Mapping) and that what we really want to do is add a DockerVolume type to s_c_l
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.
Changed to direct casting. Do you mean to import the paasta repo later as a dependency of s_c_l? I checked that other repos which uses s_c_l do not have dependency to the paasta repo (makes them more lightweight)
33a50cd
to
ad0d9b8
Compare
Add paasta names to spark driver pod annotations.
Follow up by: Yelp/service_configuration_lib#153 which is for executor pod annotations.