Page MenuHomePhabricator

[cron] Save logs to ~/dagster directory
ClosedPublic

Authored by sashank on Aug 5 2019, 11:23 PM.

Details

Reviewers
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:0c0ff0c534b5: [cron] Save logs to ~/dagster directory
Summary

Save logs to `$DAGSTER_HOME/logs/v0/repo_name'

Test Plan

Run dagit --log
Execute run
Close dagit
Check contents of `$DAGSTER_HOME/logs/experimental/repo_name'
Re-run dagit and confirm run loads properly

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sashank created this revision.Aug 5 2019, 11:23 PM
sashank updated this revision to Diff 3421.Aug 5 2019, 11:31 PM

Add setting $DAGSTER_HOME to installation steps

sashank added a reviewer: Restricted Project.Aug 5 2019, 11:33 PM

i feel like we should be explicit and error when not set instead of defaulting to ~/dagster - very possible that directory is already in use

sashank added inline comments.Aug 5 2019, 11:38 PM
python_modules/dagster/dagster/utils/__init__.py
227

lmk if there's a better place for these helper functions to live

Airflow sets up $AIRFLOW_HOME during installation, and uses the default of ~/airflow

What do you think about doing the same during our install, and removing the default in the code here?

interesting - I could be convinced otherwise but the experience that I think i would want is:

  • don't set it by default
  • error if it is not set and --log is passed without --log-dir: have error message indicate both options of using --log-dir or setting $DAGSTER_HOME
  • don't mention it in the install instructions since its only used in this one place so far
  • probably drop the v0 bit unless you think that its a really good idea - i was just throwing ideas out there

I think if more stuff depends on it it will make sense to change the behavior - but this is where i land at the moment

python_modules/dagster/dagster/utils/__init__.py
227

this seems fine for now

for example: I have dagster checked out to ~/dagster - what happens when i run this? it just starts writing stuff in to my git checkout? Do we need to have some marker file that we look for / write on first initialization? At least having it explicitly set means that a user has to navigate these problems and we can reasonably assume they have set DAGSTER_HOME To something not already used. Not sure how airflow handles this case.

sashank marked an inline comment as done.Aug 6 2019, 12:03 AM

Makes sense - I agree

I think the v0 is good for now as we experiment. We can drop it before the release if we think it's unnecessary then?

alangenfeld added inline comments.Aug 6 2019, 12:05 AM
python_modules/dagster/dagster/utils/__init__.py
233

I go back and forth on this 'v0' bit - maybe we should just do beta or something to indicate instability. v0 feels like its locking us in to a versioning scheme.

sashank updated this revision to Diff 3422.Aug 6 2019, 12:05 AM

Error if $DAGSTER_HOME is not set

sashank updated this revision to Diff 3423.Aug 6 2019, 12:06 AM

Use beta instead of v0

python_modules/dagster/dagster/utils/__init__.py
233

beta sounds much better, thanks :)

alangenfeld accepted this revision.Aug 6 2019, 12:10 AM

experimental is another good alternative if beta feels off

python_modules/dagster-graphql/dagster_graphql/cli.py
152–153

we should do a check here for if the home dir is not set and throw a more useful error message - that and/or update the help text for --log to indicate thats where we are going to go look for it

This revision is now accepted and ready to land.Aug 6 2019, 12:10 AM
sashank updated this revision to Diff 3440.Aug 6 2019, 6:07 PM

Update errors and help messages

sashank updated this revision to Diff 3442.Aug 6 2019, 6:09 PM

beta -> experimental

sashank retitled this revision from [WIP] [cron] Save logs to ~/dagster directory to [cron] Save logs to ~/dagster directory.Aug 6 2019, 6:09 PM
sashank edited the test plan for this revision. (Show Details)
Harbormaster completed remote builds in B2749: Diff 3442.