Page MenuHomeElementl

change DagsterInstance.get behavior
ClosedPublic

Authored by alangenfeld on Apr 1 2021, 7:17 PM.

Details

Summary

Our setup has matured and changed a lot since this pattern was first introduced.

The main change here is to change DagsterInstance.get to only return a DAGSTER_HOME based instance.

To maintain the gracefully / gradual getting started with dagster path, we still allow you to use dagit without it set. The change here is to be explicit that we are using a temporary directory and that DAGSTER_HOME can be used for persistence.

resolves https://github.com/dagster-io/dagster/issues/3717
resolves https://github.com/dagster-io/dagster/issues/3817

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.Apr 1 2021, 8:45 PM
Harbormaster failed remote builds in B28347: Diff 34776!
alangenfeld added a reviewer: prha.
Harbormaster returned this revision to the author for changes because remote builds failed.Apr 5 2021, 11:29 PM
Harbormaster failed remote builds in B28464: Diff 34932!
Harbormaster returned this revision to the author for changes because remote builds failed.Apr 6 2021, 3:20 PM
Harbormaster failed remote builds in B28477: Diff 34949!
python_modules/dagit/dagit_tests/test_app.py
183

is there a strong reason not to use a context manager for these tests

python_modules/dagster/dagster/core/instance/__init__.py
311

so to run dagit you must now set DAGSTER_HOME

python_modules/dagster/dagster_tests/cli_tests/command_tests/test_list_command.py
89–96

what instance do these tests run in now?

python_modules/dagit/dagit/cli.py
98–110

[1] - need to improve this message, but the goal is to make it clear to the user that they are in using dagit with a temporary instance

python_modules/dagit/dagit_tests/test_app.py
183

nope, can be switched

python_modules/dagster/dagster/core/instance/__init__.py
311

nope, that code path is [1]

python_modules/dagster/dagster_tests/cli_tests/command_tests/test_list_command.py
89–96

none, listing things in a workspace does not interact with an instance at all so its not required

this is great, we should probably write up a changelog entry

This revision is now accepted and ready to land.Apr 7 2021, 4:18 PM

I wasn't sure if this would be kosher for a minor release - I expect things shouldn't break but theres a small chance someone is depending on it. I could mention DagsterInstance.ephemeral() in the message.

Also could use some help on the copy for the dagit falling back to temp storage message

requesting review to get more takes on release sooner vs later as well as on the user facing messages

python_modules/dagit/dagit/cli.py
98–110

help here

python_modules/dagster/dagster/core/instance/__init__.py
56–57

help here

python_modules/dagster/dagster/core/instance/__init__.py
311

But it will still affect the dagster CLI, yes?

python_modules/dagster/dagster/core/instance/__init__.py
311

correct - this diff does make dagster pipeline execute require DAGSTER_HOME. It is not obvious to me if that is good, but I do think that you should get a message like the one in dagit if we decide to support ephemeral fallback. Likely it would be different since it would fallback to in-memory instead of a temp dir backed

commands that don't interact with the instance like dagster pipeline list do not require it since they do not interact with the instance

I think stuff like dagster run list is certainly better that it requires DAGSTER_HOME since you'ld just get silent empty results now

python_modules/dagit/dagit/cli.py
98–110

might be nice to tell them when the temporary directory will go away & tell them what will be ephemeral as a consequence. presumably if you do this, you can't run the daemon?

105

should probably be warnings.warn

python_modules/dagster/dagster/core/instance/__init__.py
56–57

link to the docs would be helpful.

copy refresh - support dagster pipeline execute

python_modules/dagit/dagit/cli.py
98–110

how do we feel about this [Demo Mode] framing?

python_modules/dagit/dagit/cli.py
98–110

I like this error message. I'm wary of using the word "Mode" here, because of the different way we use that word elsewhere in the system.

python_modules/dagster/dagster/core/instance/__init__.py
56–57

It could also be helpful to mention DagsterInstance.ephemeral(), if there are situations where we think users might have called this to try to get something lightweight to pass into a test.

python_modules/dagster/dagster/core/errors.py
536

seems worthy - "invariant violation" is scary imo for something many users might hit

python_modules/dagster/dagster/core/instance/__init__.py
155–160

[A]

316–321

changed some of the phrasing here

322

pretty awkward to find a place to put this line about ephemeral in this error - is API docs enough [A]? should it go up above the recommended solution?

331–340

made these more terse since we dont *require* a config file

python_modules/dagster/dagster/core/instance/config.py
32–35

updated

sandyryza added inline comments.
python_modules/dagster/dagster/core/instance/__init__.py
322

IMO what you settled on here is good or at least fine.

This revision is now accepted and ready to land.Tue, Apr 20, 3:31 PM
alangenfeld retitled this revision from [RFC] change DagsterInstance.get behavior to change DagsterInstance.get behavior.Tue, Apr 20, 4:42 PM
This revision was automatically updated to reflect the committed changes.