Skip to content

Commit

Permalink
Skip test-project image builds (#10099)
Browse files Browse the repository at this point in the history
Our dependency chain here is...really hairy. I don't particularly like
this solution but I'm prioritizing the immediate goal faster developer
velocity over cleanliness/sanity of code.

Here's our problem: we only know if we need these images built and
pushed once we know if we need to run certain Python package builds.
Without a larger refactor to switch to a two-phase build of the pipeline
(first generate a graph of our build, second compile it), it's really
hard to decide if these image builds need to run.

While I hope we eventually move toward that two-phase approach, this
takes a major shorcut generating the Python package build steps first
and letting them set a marker value in `build_for` if we need the image
built. It's bad. It's ugly. It's confusing. I think it might work?

Open to other (simple) suggestions. Otherwise, this is already a very
complicated part of our build and I'm fine temporarily increasing its
complexity if it unblocks moving onto other build improvements.
  • Loading branch information
jmsanders committed Oct 27, 2022
1 parent 3176dcc commit a24298a
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 13 deletions.
12 changes: 6 additions & 6 deletions .buildkite/dagster-buildkite/dagster_buildkite/package_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,12 +214,12 @@ def build_steps(self) -> List[GroupStep]:
else:
extra_commands_post = []

if isinstance(self.pytest_step_dependencies, list):
dependencies = self.pytest_step_dependencies
elif callable(self.pytest_step_dependencies):
dependencies = self.pytest_step_dependencies(py_version, other_factor)
else:
dependencies = []
dependencies = []
if not self.skip_reason:
if isinstance(self.pytest_step_dependencies, list):
dependencies = self.pytest_step_dependencies
elif callable(self.pytest_step_dependencies):
dependencies = self.pytest_step_dependencies(py_version, other_factor)

steps.append(
build_tox_step(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,16 @@ def build_repo_wide_steps() -> List[BuildkiteStep]:
def build_dagster_steps() -> List[BuildkiteStep]:
steps: List[BuildkiteStep] = []

# Build images containing the dagster-test sample project. This is a dependency of certain
# dagster core and extension lib tests.
steps += build_test_project_steps()

# "Package" used loosely here to mean roughly "a directory with some python modules". For
# instances, a directory of unrelated scripts counts as a package. All packages must have a
# toxfile that defines the tests for that package.
steps += build_library_packages_steps()

# Build images containing the dagster-test sample project. This is a dependency of certain
# dagster core and extension lib tests. Run this after we build our library package steps
# because need to know whether it's a dependency of any of them.
steps += build_test_project_steps()

steps += build_helm_steps()
steps += build_sql_schema_check_steps()
steps += build_graphql_python_client_backcompat_steps()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import List
from typing import List, Optional, Set

from ..images.versions import (
BUILDKITE_BUILD_TEST_PROJECT_IMAGE_IMAGE_VERSION,
Expand All @@ -8,15 +8,30 @@
from ..step_builder import CommandStepBuilder
from ..utils import BuildkiteLeafStep, GroupStep

# Some python packages depend on these images but we don't explicitly define that dependency anywhere other
# than when we construct said package's Buildkite steps. Until we more explicitly define those dependencies
# somewhere, we use these sets to track internal state about whether or not to build the test- project and
# test-project-core images.
#
# When we build the Buildkite steps for a python package that needs one or both of these images, add the
# required versions to these sets. We'll otherwise skip building images for any versions not requested here.
#
# This means you need to call `build_test_project_steps()` after you've build the other Buildkite steps that
# require the images.
#
# TODO: Don't do this :) More explicitly define the dependencies.
# See https://github.com/dagster-io/dagster/pull/10099 for implementation ideas.
build_test_project_for: Set[AvailablePythonVersion] = set()
build_test_project_core_for: Set[AvailablePythonVersion] = set()


def build_test_project_steps() -> List[GroupStep]:
"""This set of tasks builds and pushes Docker images, which are used by the dagster-airflow and
the dagster-k8s tests
"""
steps: List[BuildkiteLeafStep] = []

# Build for all available versions because a dependent extension might need to run tests on any
# version.
# Build for all available versions because a dependent extension might need to run tests on any version.
py_versions = AvailablePythonVersion.get_all()

for version in py_versions:
Expand Down Expand Up @@ -58,6 +73,7 @@ def build_test_project_steps() -> List[GroupStep]:
"BUILDKITE_SECRETS_BUCKET",
],
)
.with_skip(skip_if_version_not_needed(version))
.build()
)

Expand Down Expand Up @@ -97,6 +113,7 @@ def build_test_project_steps() -> List[GroupStep]:
"BUILDKITE_SECRETS_BUCKET",
],
)
.with_skip(skip_core_if_version_not_needed(version))
.build()
)
return [
Expand All @@ -113,6 +130,7 @@ def _test_project_step_key(version: AvailablePythonVersion) -> str:


def test_project_depends_fn(version: AvailablePythonVersion, _) -> List[str]:
build_test_project_for.add(version)
return [_test_project_step_key(version)]


Expand All @@ -121,4 +139,19 @@ def _test_project_core_step_key(version: AvailablePythonVersion) -> str:


def test_project_core_depends_fn(version: AvailablePythonVersion, _) -> List[str]:
build_test_project_core_for.add(version)
return [_test_project_core_step_key(version)]


def skip_if_version_not_needed(version: AvailablePythonVersion) -> Optional[str]:
if version in build_test_project_for:
return None

return "Skipped because no build depends on this image"


def skip_core_if_version_not_needed(version: AvailablePythonVersion) -> Optional[str]:
if version in build_test_project_core_for:
return None

return "Skipped because no build depends on this image"

0 comments on commit a24298a

Please sign in to comment.