Page MenuHomeElementl

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

Authored by cdecarolis on Jul 27 2021, 5:16 PM.

Details

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

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
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?

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.

Move version inclusion to output_identifier fxn on output context

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

Properly fail if attempting to use mapping keys with versioning.

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

Addressed locally