build completed successfully, not sure why the hook didn't publish.

Up

Up

Up

Up

Up

Addressed comments

Up

Up

Fixed nits. Re-added VersionedAssetStore abstract class so that users are not forced to implement has_asset method

Up

Up

Rebased on top of Versioned Asset Store changes

Removed VersionedAssetStore abstract class, and instead added a has_asset method to the base assetstore class.

Rebased on top of AssetStoreContext changes

LGTM! I think these changes are gonna enable the versioning workstreams in a big way.

LGTM! Fwiw @sandyryza I think exposing the construct method makes sense, if at least for the testing case that we talked about before.

to your queue

Fixed set_asset call

Migrated to using asset store context

fixed messaging on filesystem_versioned_asset_store

Up

Addressed comments. Moved version resolution logic off of the execution plan. passed mode as a param of environment config instead of mode_definition to execution plan. Got rid of outdated documentation. Assert -> check invariant.

Up

Fixed lint issue

upUp

Up

Up

Use topological steps

In D5081#138030, @sandyryza wrote:

This might work for the in-process executor, but I suspect that, for other executors, this will encounter difficulties, because the execution plans inside the steps will be constructed via a path that doesn't involve resolve_memoized_execution_plan.

More generally - ExecutionPlans are produced and consumed from a variety of locations in the codebase. If the presence of the step output versions depends on where the execution plan was created, it requires readers to trace back to where it was created in order to understand how it can be used.

I wonder if it instead makes sense to resolve step versions inside the execution plan as part of its construction.

up

Placed computed fxn to compute step output versions on execution plan using information already provided in step builder

Improved comments

black

Added additional failure mode tests. When non-str key provided, no type loader is set.

Added test for when inner type without loader is used

thanks for fix!

Looks good!

Thanks so much for fixing these up.

docs make black

Added issue tracking

Looks good. Thanks for catching!

Awesome stuff - just want to make sure I understand the breaking correctly here: A call such as Output("value", address="some_address") is no longer permitted?

Changed print_versions -> list_versions. Consolidated config option. Made list_versions logic more concise.

I'm not too familiar with ObjectStoreOperation's fxnality / how this ultimately ties back to the addressing stuff. Deferring to @yuhan here.

Got rid of some really questionable control flow.

I must have missed this one with my fix. Thanks so much for catching it! LGTM

LGTM!

Got rid of test_build_memoized_plan, was subsumed by other tests.

Fixed lint errors

Fixed issue with step execution that was preventing hydration. Changed pipeline to actually create result files. Renamed and moved files since they no longer directly test the CLI.

Up

Up

up

Up

Up

Up

Up

Fixed nits. Altered partial versioning test to fit with changes

Removed implementation-specific test. Added type checking to values in address dict

In D4658#125799, @alangenfeld wrote:

theres nothing objectionable in here but I actually think a bigger more fleshed out diff that actually uses this newly added information would be preferable.

The only thing that worries me is how this might play with the part of the system that actually caches addresses for a given version. But as long as we're intentional going forward with preserving this behavior it should be fine. The fact that steps upstream of a step with a filled-in version really helps us here.