Skip to content
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

Add an env switch to both workflows that enable the new pex based workflows #23

Merged
merged 19 commits into from
Nov 10, 2022

Conversation

shalabhc
Copy link
Contributor

@shalabhc shalabhc commented Nov 8, 2022

A new env var ENABLE_PYTHON_EXECUTABLE is added. It is unset by default and the default workflow execution is identical to the current (docker based) flow.

The current flow has two jobs: parse_workspace and Dagster Serverless Deploy. Adding conditional execution without adding a new job seems impossible in github actions. So the switch is implemented as below:

  1. Rename parse_workspace to Dagster Serverless Deploy and add new steps for pex. Add conditions to existing and new steps that look at env.ENABLE_PYTHON_EXECUTABLE.
  2. Rename Dagster Serverless Deploy to Dagster Serverless Docker Deploy. Add a condition for the job that looks at output of previous job.

In default execution both of the jobs run, identical to the current flow, just using different names:
image

When you set ENABLE_PYTHON_EXECUTABLE: 'true', only the first job runs and executes the pex steps instead of the parse-workflow steps:
image

image


jobs:
parse_workspace:
dagster_cloud_default_deploy:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than combining the steps into a single job, can we put the condition on the job itself and then have 3 top-level jobs?

e.g.

jobs:
  parse_workspace:
    if: env.ENABLE_PYTHON_EXECUTABLE != 'true'
    ...
  dagster_cloud_build_push:
    if: env.ENABLE_PYTHON_EXECUTABLE != 'true'
    needs: parse_workspace
    ...
  dagster_cloud_python_deploy:
    if: env.ENABLE_PYTHON_EXECUTABLE == 'true'
    steps:
      - name: Checkout
         ...
      - name: Build and deploy Python executable
        ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I tried first, but unfortunately it is not possible to access env in the job level. It is only available in steps. actions/runner#1189 (comment)

I looked for other possible ways to use a variable that can be used to switch the job, but there doesn't seem to be any.

Maybe we could create a new 'switcher' job that runs first and writes a job output which is then used in subsequent job's if, but running another job adds another 10s to spin up a new container so not sure if it is worth it.

@shalabhc shalabhc requested a review from prha November 9, 2022 23:51

- name: Build and deploy Python executable
if: env.ENABLE_PYTHON_EXECUTABLE == 'true'
uses: dagster-io/dagster-cloud-action/actions/build_deploy_python_executable@pex-testing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note, we will probably have to keep this tag around forever (lest we break users who opt into this), since they will have cloned this into their own repo.

consider changing this to something like v0.1 or something like that, that we feel comfortable moving?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. v0.1 already exists and I don't want to move it to HEAD where most of the newer pex changes will land. should i make this v0.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, changed to @pex-v0.1 so this tag is independent of the main releases for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's acceptable to also just do v0.1.10 or something like that. We should be moving the v0.1 tag along with the minor versions, as long as there are no breaking changes.

If we want to make backwards incompatible changes, we could bump to v0.2 and then all the existing repos would need to change the tag to take advantage of the newer stuff.

@shalabhc shalabhc requested a review from prha November 10, 2022 00:26
@shalabhc shalabhc merged commit b2ebc02 into dagster-io:main Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants