Page MenuHomeElementl
Differential D9085

[memoization 8/n] make default fs io manager memoizable
ClosedPublic
Actions

Authored by • cdecarolis on Jul 27 2021, 5:16 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 28, 2:03 PM
Unknown Object (File)
Tue, Jan 24, 6:13 AM
Unknown Object (File)
Fri, Jan 13, 4:42 PM
Unknown Object (File)
Dec 13 2022, 10:26 PM
Unknown Object (File)
Nov 19 2022, 5:10 PM
Unknown Object (File)
Nov 15 2022, 6:43 AM
Unknown Object (File)
Nov 15 2022, 6:43 AM
Unknown Object (File)
Nov 15 2022, 6:43 AM
View All Files
Subscribers
None

Details

Reviewers
• sandyryza
alangenfeld
Commits
R1:1f60d6a581e8: [memoization 8/n] make default fs io manager memoizable
Summary

This diff makes the default fs io manager subclass MemoizableIOManager. This further decreases the friction users will encounter when they try to use memoization.

If this goes through, we can probably get rid of versioned_fs_io_manager. The question for me is whether we might want to preserve it for backcompat, or given the experimental-ness of all the memoization stuff, just force people to migrate.

Test Plan

Added additional unit test for memoization mode

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• cdecarolis created this revision.Jul 27 2021, 5:16 PM
• cdecarolis updated this revision to Diff 42379.Jul 27 2021, 5:34 PM

Up

Harbormaster failed remote builds in B34281: Diff 42376!Jul 27 2021, 5:58 PM
Harbormaster completed remote builds in B34284: Diff 42379.Jul 27 2021, 6:00 PM
• cdecarolis requested review of this revision.Jul 27 2021, 6:00 PM
• cdecarolis mentioned this in D9059: [memoization 2/n] provide default base dir for versioned fs io manager.Jul 27 2021, 8:03 PM
• cdecarolis edited the summary of this revision. (Show Details)Jul 28 2021, 6:47 PM
• sandyryza accepted this revision.Jul 30 2021, 3:17 PM

I think this makes sense. I feel like there might be an edge case out there where this causes trouble, but having trouble thinking of it. With this, are we able to get rid of VersionedPickledObjectFilesystemIOManager?

python_modules/dagster/dagster/core/storage/fs_io_manager.py
105

Not a big deal, but this might be a little weird as the only piece of info logged by this IOManager.

This revision is now accepted and ready to land.Jul 30 2021, 3:17 PM
alangenfeld added inline comments.Aug 2 2021, 3:55 PM
python_modules/dagster/dagster/core/storage/fs_io_manager.py
101–104

are dynamic outputs under test for this stuff at all?

103

these are ultimately user provided strings - do we enforce the contents of said strings well enough yet given they are used for FS paths?

• yuhan mentioned this in D8963: [memoization 3/n] move core memoization logic to live on the execution plan.Aug 2 2021, 5:18 PM
• cdecarolis added inline comments.Aug 3 2021, 9:57 PM
python_modules/dagster/dagster/core/storage/fs_io_manager.py
101–104

nope - planning to add

103

definitely not - Do we have a validation code path for externally-provided run ids? I could key into that as well.

• cdecarolis updated this revision to Diff 42789.Aug 4 2021, 2:27 AM

Move version inclusion to output_identifier fxn on output context

Harbormaster completed remote builds in B34597: Diff 42789.Aug 4 2021, 2:51 AM
• cdecarolis added inline comments.Aug 4 2021, 3:11 AM
python_modules/dagster/dagster/core/storage/fs_io_manager.py
103

@alangenfeld: I added https://dagster.phacility.com/D9204 to address validation

• cdecarolis retitled this revision from [memoization 6/n] make default fs io manager memoizable to [memoization 8/n] make default fs io manager memoizable.Aug 4 2021, 3:11 AM
• cdecarolis updated this revision to Diff 42806.Aug 4 2021, 11:08 PM

Properly fail if attempting to use mapping keys with versioning.

Harbormaster failed remote builds in B34608: Diff 42806!Aug 4 2021, 11:31 PM
• cdecarolis updated this revision to Diff 42808.Aug 5 2021, 12:41 AM

up

Harbormaster completed remote builds in B34609: Diff 42808.Aug 5 2021, 1:27 AM
alangenfeld added inline comments.Aug 9 2021, 7:31 PM
python_modules/dagster/dagster/core/execution/context/output.py
165

you need to keep this around for user defined IOManagers right that may still be using this right? I think this needs to get kept around & deprecated instead of a hard breaking change

188–207

these cast can get cleaned up if you do if checks (+ check.failed) so the static analyzer knows they are not None. You may need to capture local references instead of self. since the analyzer assumes that self may change at any fn invocation

• cdecarolis marked 2 inline comments as done.Aug 9 2021, 10:34 PM

Addressed locally

Closed by commit R1:1f60d6a581e8: [memoization 8/n] make default fs io manager memoizable (authored by Chris DeCarolis <cdguitar817@gmail.com>). · Explain WhyAug 9 2021, 10:51 PM
This revision was automatically updated to reflect the committed changes.
Chris DeCarolis <cdguitar817@gmail.com> added a commit: R1:1f60d6a581e8: [memoization 8/n] make default fs io manager memoizable.

Revision Contents
Changeset List

PathSize
python_modules/
dagster-test/
dagster_test/
toys/
2 lines
4 lines
dagster/
dagster/
core/
definitions/
8 lines
execution/
context/
62 lines
storage/
10 lines
4 lines
3 lines
dagster_tests/
core_tests/
storage_tests/
39 lines
17 lines
libraries/
dagster-aws/
dagster_aws/
s3/
2 lines
dagster-azure/
dagster_azure/
adls2/
2 lines
dagster-gcp/
dagster_gcp/
gcs/
2 lines

Diff 42843

View Options

python_modules/dagster-test/dagster_test/toys/asset_lineage.py

Loading...
View Options

python_modules/dagster-test/dagster_test/toys/error_monster.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/pipeline.py

Loading...
View Options

python_modules/dagster/dagster/core/execution/context/output.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/fs_io_manager.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/mem_io_manager.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/memoizable_io_manager.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_fs_io_manager.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_io_manager.py

Loading...
View Options

python_modules/libraries/dagster-aws/dagster_aws/s3/io_manager.py

Loading...
View Options

python_modules/libraries/dagster-azure/dagster_azure/adls2/io_manager.py

Loading...
View Options

python_modules/libraries/dagster-gcp/dagster_gcp/gcs/io_manager.py

Loading...