Looks like this is passing in bk - not sure why it's saying it failed.

up

Used string method by convention

up

Added return to docstring

This looks largely good, although I think it would also be affected by https://dagster.phacility.com/D4480. Would you be able to refactor to take that into account?

I think a lot of the most recent changes to https://dagster.phacility.com/D4480 will break this, unfortunately. Most notably the renaming of the config_value parameter,

Renamed config_value to run_config, added an additional test for resourced solids.

Added tests to test_required_resources.py to ensure that default version was correct, and version was being passed all the way to resource definition.

In D4481#121386, @sandyryza wrote:

When I run into small bugs like this, I normally try to include a test that would have caught the bug in the first place. Might be easy to add or augment something in test_resource_definitions.

@sandyryza I needed to make one change to create_run_for_pipeline.py - want to make sure this doesn't screw up what you were working on.

Made helper function to avoid directly accessing _composition_stack global.

In D4457#120776, @yuhan wrote:

you should be able to get both mode and run_config from execution_plan.pipeline_def

In D4457#120766, @sandyryza wrote:

I actually think we should do the opposite (don't provide an execution plan, but provide a pipeline def, run config, and mode); there is no convenient way of obtaining mode or environment_config, both of which will be necessary in order to resolve config + resource versions.

Per Slack conversation - if we need the run_config and mode to resolve versions, is there any issue with just passing those in to resolve_step_versions directly? If we build the ExecutionPlan inside resolve_step_versions, we'll also need to build it inside resolve_unmemoized_steps. While this isn't a performance concern, I think it does hurt readability and debuggability a bit.

I actually think we should do the opposite; there is no convenient way of obtaining mode or environment_config, both of which will be necessary in order to resolve config + resource versions.

This makes sense. It was kind of wonky having both options, and added a lot of unnecessary complication.

This approach makes sense to me. However, I do feel like some of these helper functions should probably live in some util file or something (like join_and_hash, etc)

documentation up

Changed default value of external input version loader to None. Added experimental warning for _external_version_fn

Made warning message easier to read. Improved tests. Pending invocations tracked at the composition context level.

Fxn naming changes. Documentation.

Up

Combined hash and join helper methods. Moved resolution of step versions to its own function on the dagster instance. Expanded testing to verify correct versions being produced.

Added public loader_version property.

Made loader version a public property

Changed parameter to solid_version. Refactored tags and hooks to use solid.

In D4404#119477, @yuhan wrote:

would it make more sense to have a version param instead of solid?

Good stuff. Makes sense to me.

@alangenfeld just to provide some context, I use this to pass version information from solid when computing the version of a step. I figured since solid was already supplied as an input to the __new__ method, this was the least intrusive way to do that. Additionally since ExecutionSteps aren't persisted, it wouldn't affect run storage, etc.

Fixed functionality

Made it explicit that "no version provided" sentinel is the None value

Up

Up

In D4405#119352, @yuhan wrote:

Is it going to be used by dagit/graphql flow only or is it also going to be used by api and cli? More specifically, is this tag required for execution that dagster requires this tag when executing a memorized pipeline?
In the current system, only the RESUME_RETRY_TAG is part of the execution logic but only in dagit/gql flow.
Other tags are primarily for storage purpose, where we don't use them in the core execution logic. For example, as for the STEP_SELECTION_TAG and SOLID_SELECTION_TAG, the reason why we use tags is for dagit to easily grab the user's selection value (the alternative was to persist them in the pipeline run table, but because we were not confident about the data model and didn't want to worry about potential data migration, we ended up putting them in tags for the short term)

Just wanted to give you context about how we currently use tags and how we think about persistency -- which are worth thinking ahead. But either way, persisting the info in tags at least for the short term sounds good!

Added tests, and made default computed version None (we're using None as our sentinel value for no versions provided).

Checked that tags exist

In D4405#119340, @yuhan wrote:

whats the plan of using this tag?

In D4341#118702, @alangenfeld wrote:

cache_scope not a great name either - but gets at the idea. To be clear in my head all it does is get hashed in to compute the final version_hash of a step/solid in the same way that the run_config will or the input values in downstream solids

In D4341#118699, @alangenfeld wrote:

Under the current system, the logical object should probably be the step output

The problematic case now is 2 different solids with manually the same version string, aliased to the same name such that the output handles are the same in the two different pipelines so collide in storage.

@solid(version='v1')
def return_one():
  return 1

@solid(version='v1')
def return_two():
  return 2

@pipeline
def pipeline_a():
  return_one.alias('emit_number')()

@pipeline
def pipeline_b():
  return_two.alias('emit_number')()

which is certainly pretty contrived - but i think represents a real rare event that would be super hard for a user to figure out if they hit it

I have this gut feeling that we will want some sort of namespacing or something that can control which pipelines participate in the same pool of memoized results. Should it just be pipeline scoped? Should pipeline have a version such that you can "blow away the whole cache" so to speak by bumping that number?

Rebased onto master

Made StepInput version computed, and propagated up to ExecutionStep version as a result.

up

LGTM!

Updated to take config_value, and not context parameter

Updated to reflect changes to DagsterTypeLoader. Reformatted via make black.

Added documentation. Changed version parameter to loader_version to reflect its usage more effectively. Reflected that external_version_fn will take in extra parameters. Rebased onto master.

Up

Implementation for hooks and tags, more comprehensive tests. Flushing of stack upon checking whether invoked.

More descriptive error messages, accounting for tags and hooks.

Formatting up

Added test

Flaky

Flakey build failures.

LGTM