[memoization 13/13] revamp memoization docs
Needs ReviewPublic
Actions

Authored by • cdecarolis on Aug 10 2021, 10:05 PM.

Details

Reviewers

• sandyryza
alangenfeld
• yuhan

Summary

This attempts a complete revamp of the memoization tutorial. Since a lot of the boilerplate around using memoization is gone now, the old format doesn't make as much sense.

I'm thinking it actually makes more sense to have a memoization overview page than a tutorial at this point. I tried to go in that direction by using more self-contained code examples, and revamping into smaller, more targeted sections that people can search for specific information in.

In addition to general cleanup, I think the biggest question for me is how to show people memoization "working". In some sense, it might not even be necessary to do this, but it would probably be useful for initial sanity checking purposes.

Another note: in this current iteration, I have removed mention of the list_versions CLI. Given that we have relatively low CLI traffic, it seemed mis-targeted. I'm open to other opinions around that though cc @sandyryza .

Test Plan

will add unit tests for code examples.

Diff Detail

Repository

R1 dagster

Branch

tutorial_revamp_memoization

Lint

Lint Passed

Unit

No Test Coverage

Event Timeline

• cdecarolis created this revision.Aug 10 2021, 10:05 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 10 2021, 10:28 PM

Harbormaster failed remote builds in B34640: Diff 42854!

Harbormaster completed remote builds in B34641: Diff 42855.Aug 10 2021, 11:02 PM

• cdecarolis requested review of this revision.Aug 10 2021, 11:02 PM

• cdecarolis retitled this revision from [memoization 12/12] revamp memoization tutorial to [memoization 13/13] revamp memoization tutorial.Aug 11 2021, 12:20 AM

• cdecarolis retitled this revision from [memoization 13/13] revamp memoization tutorial to [memoization 13/13] revamp memoization docs.Aug 11 2021, 2:04 AM

• cdecarolis edited the summary of this revision. (Show Details)

• cdecarolis edited the test plan for this revision. (Show Details)

• cdecarolis added inline comments.Aug 11 2021, 5:42 AM

docs/content/guides/dagster/memoization.mdx
23–28	cc @alangenfeld , I know we talked about this not being the best approach. I kept it here as a placeholder for now. I do think our forward example should track code. It might be best to just caveat that it's not gonna catch everything, and suggest an approach that can be a bit more thorough?
55	Does anyone think it's necessary to show "memoization in action"? I feel like the previous iteration focused too much on that, making it feel more like a demo than a guide, but maybe I've gone too far in the other direction here. One option would be showing logs in dagit before and after memoization (before we see step events, after we see no step events, after version change we see some step events).
60	To compliment this section, I want to add a "how memoization works" page, that explains in more literal detail how each version is constructed. Where would be the best place for that? cc @yuhan
70	This sentence is very confusing, and I'm thinking can benefit from a demonstrable example.
84	I need to actually link to the implementation here.
88	Need to add a code example for this.
92	Need to add a code example for this as well.

alangenfeld added inline comments.Aug 11 2021, 4:01 PM

docs/content/guides/dagster/memoization.mdx
23–28	ya the comment just needs to be expanded to clearly communicate what it captures and what its limitations are. I think i am fine with whatever as long as its very clearly communicated

@cdecarolis would you mind submitting this as a github PR? Submitting docs changes as a github PR lets us preview the build docs using Vercel.

• sandyryza added inline comments.Aug 12 2021, 3:10 PM

docs/content/guides/dagster/memoization.mdx
9	"example" -> "guide"
11	Suggestion for replacing the second sentence in a way that allows avoiding the verb "to be" and ties directly to the first sentence: Versions allow users to express to Dagster whether outputs have changed and thus require re-computation.
23	I find this name a little strange - it's not directly tracking changes, it's just tracking the op's source code and leaving it up to Dagster to determine whether the version has changed. Might be clearer as `OpSourceHashStrategy` or something.
55	I don't think so.
examples/docs_snippets/docs_snippets/guides/dagster/memoization/memoization.py
1	Nice on including imports with each code snippet.

Address comments. Add tests for guide. Add tags to execute_in_process. Add resource_key to version strategy s.t. you can differentiate between resources if you choose.

cc @sandyryza since this stack is making heavy use of the stacked diffs workflow atm, I think switching to a PR might be a bit rough. i'll definitely put up screenshots though.

Harbormaster failed remote builds in B34650: Diff 42867!Aug 13 2021, 1:44 AM