User Details
- User Since
- Mar 20 2019, 8:23 PM (202 w, 4 d)
- Roles
- Administrator, Disabled
Jun 21 2021
@sandyryza Yeah I actually find that reasoning pretty compelling. Do you think we should support the old format whenever?
Jun 19 2021
Jun 18 2021
Definitely want to kick-test the dict-style passing for dogfooding at a minimum. I have meetings starting in about 5 mins but I do have a preference for the dict-style approach for a few reasons. Will follow up later
Jun 11 2021
IMO if the purpose to support "pipeline_name: mode_name" in dagit displays we could do that at the dagit/graphql layer exclusively rather than inject the notion of a suffix into the greenfield job concept
It seems odd to provide this and not just a name param
Jun 10 2021
Super exciting stuff. Sandy Ryza: The Ontology Slayer
Jun 9 2021
If this is just a temporary exigency to enable CRAG and we will eventually remove mode, maybe the argument to the ModeDefinition constructor should __config_mapping_for_job_compat to make it clear that we're not support it as a public API and people shouldn't worry about the weird implications of it on shared modes and other issues?
Jun 3 2021
I'm a bit more wary of allowing users to do that. This is "special" since it is a "system" resource that is not directly reference by a context. (i.e., the user always goes through the intermediate logging API rather than direct against the resource)
Yeah I think with this one might need some special rule for "logger resources" that they are automatically added to every solid in an execution
One nice thing we could do here would be to make it easy to filter down log messages in the structured viewer to specific loggers. That could actually be a pretty nice feature.
May 26 2021
I think this is a fine incremental improvement. To me the biggest question (which this begins to address in some ways) is testing complex resources in a nice way. Would love to see how this feels on dogfooding/demo pipelines
May 24 2021
can you spin this on github so we get vercel previews?
May 7 2021
I think we can just remove it from all docs and examples and leave it as undeprecated since the cost of keeping it around is low
May 5 2021
May 4 2021
I'll let @alangenfeld accept, but I'm very supportive of this change and excited to see it land!
Apr 30 2021
Actually duh that is in the daemon process rather than dagit.
Re: th
I think this is potentially an exciting improvement (despite some initial hesitancy about the magic) and not just for testing. There are other benefits as well:
One last question on this one: what happens if someone passes a non-json serializable value in the dictionary that gets automatically coerced to JsonMetadataEntryData. Do we have a clear error message? A test of this would be good.
Apr 29 2021
This looks great. A huge improvement. One minor thing is that I think we should add a union or marker interface identify all entry data classes for more concise documentation (probably some code wins too?).
Apr 28 2021
Convo with Sandy on slack:
Chatted with sandy briefly and got to agreement. This content should eventually be massaged and resurfaced in a guide or post. I archived it here: https://docs.google.com/document/d/1B7fEOM8Ga03NbDwSu29LF7--JDri8JPGV1YBeE4rIIE/edit
Apr 27 2021
No problem on the thrash. think I'm the one that started (or at least continued) the bikeshedding party! We should probably use the term "values" instead of "variables" in examples, tests, and docs, but I think this is good.
Cool. Agree with your reasoning on the _def suffix! Was just spitballing. Req'ing for q mgmt.
Yeah that name in particular I find very confusing. Config is the way that you specify how something is constructed, rather than the output of that construction process. It also makes it sound like a property or feature of the config system, rather than the other way around. I'm concerned about the reaction of "Oh a config resource. I guess this means the config system has a resource concept as well?"
Oh I misinterpreted/read. So you are proposing introducing a new class EventMetadata?
removing myself to reduce noise. please re-add once this is ready for review!
Given that we're introducing the short form and the assumption is that the long form will used way way less, I don't think that renaming the long form is worth the thrash.
Reflecting on this I think the major con of this approach is that the term "variable" (1) makes it feel like an integrated feature as opposed to a user-space feature that a user could easily implement on their own, (2) it occupies the term "variable" which could potentially be used for a feature in the future and (3) it could cause confusion with Airflow variables.
Apr 26 2021
@sandyryza @yuhan What do you think of this approach?
Yes effectively that with a little sugar and an approachable framing.
Not suggesting a reserved keyword. This is mostly about naming and norms. I think people are likely to understand intuitively what a variables_resource is used for and the config variant is harder to understand. This also allows the use of the variables resource without specifying a config schema.
This name is definitely a bit tough. If I wanted to get global variables into the pipeline make_config_resource wouldn't really lead me to it.
Apr 23 2021
whats the plan with this?
seems like a good compromise
Apr 22 2021
should be really straightforward to implement invokable on top of this
One other thing worth noting here is that since we are passing inputs directly and getting out outputs directly resource requirements from i/o managers should be ignored
I think it's a great direction and solves the problem quite elegantly.
Agree with alex that we should focus on direct solid execution only
I'd like to have a nice dagstermill example in the repo, and this was the best one. Other than that I support deleting.
Apr 21 2021
great stuff. consider final comment but big improvement
that did it! thanks
This is a great overall. Just a few suggestions.
The preview link looks out-of-date?
Apr 20 2021
Yes I think this is a good idea
Apr 19 2021
This is a great strat.
per-offline discussion agree with decision that InputProps or similar doesn't clear the cost-benefit bar right now
One reason I like the named "InputProps" is that it felt additive, rather than the single source of truth for definitions. My concern here is that input defs would no longer be a single source of truth. The current state of things is definitely annoying so this is not a veto or anything. Just raising the concern.
We also need to determine the fate of the dagit "config nub" with this change. cc: @bengotow
Apr 15 2021
minor changes but bouncing back to your queue
Super excited for this change. Big step forward. Thanks @cdecarolis for working through this. Will let others approve.
Apr 14 2021
I'd love to get this in for tmrw's release. Feel free to take stab at my proposed refactor or not. Your discretion.
Apr 13 2021
To be clear in vanilla solids you still have to raise these right?
Apr 12 2021
Very exciting.
Apr 10 2021
Apr 4 2021
Mar 30 2021
Mar 25 2021
Mar 22 2021
Slightly concerned about possible weird aesthetics of whitespace depending on how the docstrings are slurped and how we handle whitespace on the client. But given that we do this for inputs it makes sense
Mar 19 2021
Mar 18 2021
Having both feels very painful. Why can't they be consolidated?
Mar 17 2021
I think this is good and we should highlight it in the release notes. "You no longer need to use DagsterPythonObjectType or make_dagster_type_usable_as_WAIT_WHY_DO_I_NEED_TO_TYPE_THIS"
ah ok. curious to hear @prha's thoughts on the diff btwn those two
q mgmt. this is very stale. if it is still in reviewable state bounce back and i'll review
I think the context construction pathways are so intricate and tied to internal implementations that they should be considered private
Yeah I think the core point of contention is whether or not the constructor for contexts should be considered private. I think they should. We could even go the point where we change *Context classes to be abstract classes only.
I support landing it. Can we add docs in the same diff (or another one concurrently?)
I think this is good. My final question is what we should default the instance to
not perfect but better than nothing