Page MenuHomePhabricator

Include optional docker image name in RepositoryPythonOrigin, for use in certain executors
ClosedPublic

Authored by dgibson on Tue, Nov 3, 10:31 PM.

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Tue, Nov 3, 11:36 PM
Harbormaster failed remote builds in B20761: Diff 25164!

Not sure what to do about the origin change :/ Can't wait until that's no longer a thing

python_modules/dagster/dagster/core/definitions/reconstructable.py
29–40

hmm wonder how you could get to explicit opt-in with a separate type or something that requires the container image be set. That way you can assert up front that whatever your image scheme is is working properly - such as ensuring the environment variable containing the image is set.

might make sense to model this as a context or something since I may want to know what git branch / commit checkout the code is at in the same way i might want to know what image i am running in. Maybe a bunch of optionals is an ok way to capture that.

python_modules/dagster/dagster_tests/api_tests/test_launch_scheduled_execution.py
288 ↗(On Diff #25173)

🥴

there are workarounds we can employ using whitelist_for_persistence to control the serialization format, in this case drop keys for the new fields if value is None so it doesn't change the output string and therefor hash

thats if we even need to bother depending on our 0.10.0 plans

Bit of a hack to keep the origin ID stable until 0.10.0 when it will no longer be a factor

dgibson retitled this revision from What it would look like if we let you specify a docker container name as part of your repo rigin and use it in the docker executor to Include optional docker image name in RepositoryPythonOrigin, for use in certain executors.Wed, Nov 4, 8:45 PM
This revision is now accepted and ready to land.Thu, Nov 5, 3:45 PM
python_modules/dagster/dagster_tests/api_tests/test_launch_scheduled_execution.py
288 ↗(On Diff #25173)

rebase and add test coverage - just went ahead and rebased on top of the breaking change for 0.10.0 that makes it safer to change origins

add some more test coverage

This revision was landed with ongoing or failed builds.Fri, Nov 20, 10:33 PM
This revision was automatically updated to reflect the committed changes.