Page MenuHomePhabricator

Uses run tags instead of external pipeline tags for user defined k8s config.
ClosedPublic

Authored by bob on Oct 23 2020, 8:51 PM.

Details

Summary

Run tags from the Dagit playground may contain user defined k8s config, but currently on the tags on the pipeline definition (external_pipeline.tags) are being parsed for user defined k8s config. With this change, all run tags are parsed for user defined k8s config.

Notes
For testing, I chose the path of mocking the k8s API client. This required a bit of refactoring on K8sRunLauncher and CeleryK8sRunLauncher to allow dependency injection for a k8s API client. The tests then use a mock to record the k8s job that was given to the k8s API client.

Test Plan

buildkite

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

bob requested review of this revision.Oct 23 2020, 9:08 PM

looks good - just needs some tests

python_modules/libraries/dagster-k8s/dagster_k8s/launcher.py
240

not sure you need the frozentags wrapper - think its already frozen

This revision now requires changes to proceed.Oct 23 2020, 9:19 PM
  • Prunes buildkite steps to only include integration tests.
  • Adds dependency injection for k8s client on dagster-k8s run launcher.
  • Adds dependency injection for k8s client on dagster-k8s run launcher.
  • Fix the pruned buildkite pipeline. 0
  • Prunes buildkite steps to only include integration tests.
  • Adds dependency injection for k8s client on dagster-k8s run launcher.
  • Fix the pruned buildkite pipeline. 0
  • Fixes pruned buildkite pipeline. 1
  • Adds test for user defined k8s config in run tags.
  • Adds test for celery-k8s run launcher.
  • Runs isort.
  • Restores buildkite pipeline.
  • Prunes buildkite steps to only include integration tests.
  • Adds dependency injection for k8s client on dagster-k8s run launcher.
  • Fix the pruned buildkite pipeline. 0
  • Fixes pruned buildkite pipeline. 1
  • Adds test for user defined k8s config in run tags.
  • Adds test for celery-k8s run launcher.
  • Runs isort.
  • Restores buildkite pipeline.
  • Fixes lint problems.
  • Prunes buildkite pipeline.
  • WIP: Bypass loading k8s config.
  • Undo pruning of buildlkite pipeline.
  • WIP: Use tmp file for kubeconfig.
  • WIP: Adds dagster-test as testenv dependency for dagster-celery-k8s.
  • WIP: Fixes unicode string issue with py2, and adds missing param for dagster-celery-k8s test.
  • WIP: Fixes unicode string issue with py2, and adds missing param for dagster-celery-k8s test.
  • WIP: Adds dagster-aws as testenv dependency for dagster-celery-k8s.
  • WIP: Adds library testenv dependencies for dagster-celery-k8s.
  • WIP: Adds library testenv dependencies for dagster-celery-k8s.
  • WIP: Adds library testenv dependencies for dagster-celery-k8s.
  • WIP: Adds library testenv dependencies for dagster-celery-k8s.
bob marked an inline comment as done.
  • Moves temporary kubeconfig into a fixture.
python_modules/libraries/dagster-k8s/dagster_k8s/launcher.py
240

Thanks for pointing that out. run.tags is actually just a python dict :0

so frozentags is still needed here

bob added reviewers: alangenfeld, johann.

nice

python_modules/libraries/dagster-celery-k8s/tox.ini
8–16 ↗(On Diff #25130)

this seems unnecessary

python_modules/libraries/dagster-k8s/dagster_k8s_tests/unit_tests/test_launcher.py
39

there has to be a better way to get a dummy external pipeline to launch that doesn't require all the tox thrash

python_modules/libraries/dagster-k8s/dagster_k8s_tests/unit_tests/test_launcher.py
39

should be able to use reconstructable, external_pipeline_from_recon_pipeline, and an @pipeline defined in this file

  • Refactors out dagster_test.
  • Refactors out dagster_test.
This revision is now accepted and ready to land.Wed, Nov 4, 5:00 PM

Cleaning and rebase.

  • Prunes buildkite steps to only include integration tests.
  • Adds dependency injection for k8s client on dagster-k8s run launcher.
  • Fix the pruned buildkite pipeline. 0
  • Fixes pruned buildkite pipeline. 1
  • Adds test for user defined k8s config in run tags.
  • Adds test for celery-k8s run launcher.
  • Runs isort.
  • Restores buildkite pipeline.
  • Fixes lint problems.
  • Prunes buildkite pipeline.
  • WIP: Bypass loading k8s config.
  • Undo pruning of buildlkite pipeline.
  • WIP: Use tmp file for kubeconfig.
  • WIP: Adds dagster-test as testenv dependency for dagster-celery-k8s.
  • WIP: Fixes unicode string issue with py2, and adds missing param for dagster-celery-k8s test.
  • WIP: Adds library testenv dependencies for dagster-celery-k8s.
  • Moves temporary kubeconfig into a fixture.
  • Refactors out dagster_test.
  • Uses repository location origin for fake external pipeline.
bob marked 2 inline comments as done.Wed, Nov 4, 6:10 PM