Page MenuHomeElementl

object manager
ClosedPublic

Authored by sandyryza on Dec 4 2020, 9:05 PM.

Details

Summary

This renames AssetStore to ObjectManager and has it inherit from InputManager and OutputManager.

ObjectManagerDefinition inherits from IInputManagerDefinition and IOutputManagerDefinition.

This diff maintains compatibility with the prior AssetStore naming. That will allow us to update all the callsites in a followup diff.

Depends on D5427.

Test Plan

bk

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 4 2020, 10:12 PM
Harbormaster failed remote builds in B22245: Diff 27022!
python_modules/dagster/dagster/core/definitions/output.py
51

The old names will be removed in a followon diff.

python_modules/dagster/dagster/core/storage/asset_store.py
3

In a followup, everything inside this file will disappear.

python_modules/dagster/dagster/core/storage/object_manager.py
61

Still need to add tests and examples.

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 5 2020, 12:01 AM
Harbormaster failed remote builds in B22264: Diff 27043!
python_modules/dagster/dagster/core/execution/context/system.py
314–318

looks more like a get_object_manager

python_modules/dagster/dagster/core/execution/plan/inputs.py
130

again get_output_manager -> get_object_manager?

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 5 2020, 8:11 PM
Harbormaster failed remote builds in B22283: Diff 27066!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 6 2020, 7:03 PM
Harbormaster failed remote builds in B22293: Diff 27078!

This seems like a great step forward.

Our last discussion will be to figure out the root input/subselection case :-)

But like I've said on the previous diffs I think this feels like a pretty fantastic base layer to build on

python_modules/dagster/dagster/core/execution/context/system.py
499

oof that is confusing. should we reconsider renaming run_id on OutputContext to encode that?

schrockn requested changes to this revision.Dec 7 2020, 1:00 PM

just requesting for tests!

fortnitecarlton.gif (270×480 px, 3 MB)

This revision now requires changes to proceed.Dec 7 2020, 1:00 PM
python_modules/dagster/dagster/core/execution/context/system.py
499

My thinking was that, if someone thinks deeply about it, this is the only reasonable interpretation:

  • If the OutputContext is referenced handle_output call, the run_id will always be the run_id of the run that's using the context, so no confusion there.
  • If the OutputContext is referenced from a load call, the run_id will be accessed via load_context.upstream_output.run_id. If the upstream output was produced by an upstream run, it seems most sensical to expect that the run_id attached to it would correspond to that run.

That said, we shouldn't necessarily expect users to think deeply about it. So I'm very open to changing.

Do you have thoughts on a name? Maybe "source_run_id"? If we don't think load_context.upstream_output.source_run_id is has too much redundancy?

great point run_id seems good then

Sent via Superhuman ( https://sprh.mn/?vip=schrockn@elementl.com )

schrockn added inline comments.
python_modules/dagster/dagster_tests/core_tests/storage_tests/test_object_manager.py
184–185

nit: change key from "store"?

This revision is now accepted and ready to land.Dec 7 2020, 11:58 PM
This revision was automatically updated to reflect the committed changes.