Page MenuHomePhabricator

sandyryza (Sandy Ryza)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 3 2020, 4:04 PM (18 w, 4 d)

Recent Activity

Today

sandyryza updated the diff for D4087: nicer error on failure to load repository in external process.

up

Tue, Aug 11, 9:38 PM
sandyryza added inline comments to D4087: nicer error on failure to load repository in external process.
Tue, Aug 11, 9:33 PM
sandyryza accepted D4157: better configured error messages & docs.

This looks great.

Tue, Aug 11, 8:02 PM
sandyryza updated the diff for D4087: nicer error on failure to load repository in external process.

up

Tue, Aug 11, 7:51 PM
sandyryza added inline comments to D4151: Throw from sync_get_external_execution_plan rather than relying on callsites to check error data.
Tue, Aug 11, 4:46 PM
sandyryza accepted D4070: renamed InMemoryIntermediatesManager and IntermediateStoreIntermediatesManager.

LGTM!

Tue, Aug 11, 3:43 PM
sandyryza added a comment to D4021: heterogeneous compute lakehouse example.

This is looking close - I added a few more comments,.

Tue, Aug 11, 3:42 PM
sandyryza abandoned D4106: Update CHANGES.md about config removal.
Tue, Aug 11, 3:38 PM
sandyryza added a comment to D4131: migration guide only includes major releases.

What concerned me about doing structuring the migration guide at the major version level was that, if someone is migrating from, say, 0.8.5 to 0.9.0, the migration guide tells about a bunch of changes / deprecations from 0.8.1-4 that aren't relevant to them. Which makes migration scarier and more confusing.

Tue, Aug 11, 3:34 PM

Yesterday

sandyryza added inline comments to D4087: nicer error on failure to load repository in external process.
Mon, Aug 10, 5:25 PM
sandyryza accepted D4098: (Depends on D4070) Incorporated IntermediateStore features into IntermediateStorage, fixed tests and subclasses accordingly..

This looks great! My one lingering concern is about whether we need to maintain compatibility with the TypeStoragePlugin interface for now.

Mon, Aug 10, 5:09 PM

Thu, Aug 6

sandyryza closed D4113: [docs] dagit overview.
Thu, Aug 6, 1:00 AM
sandyryza committed R1:59fa6a43e599: [docs] dagit overview (authored by sandyryza).
[docs] dagit overview
Thu, Aug 6, 1:00 AM
sandyryza added a comment to D4103: implement configured logger, executor.

Regarding the open questions you left inside the config_map functions, I don't have direct experience with logger / executor configuration edge cases. However, my high level understanding of how it should work is:

  • If we've successfully processed config, every logger/executor/resource entry in the run config dict will correspond to a logger/executor/resource definition in the mode.
  • The reverse is not true. I.e. not all definitions in the mode will correspond to an entry in the run config dict. For resources, it's not true because some resources don't require configuration, in which case they don't need to be mentioned in the run config dict. For executors, it's not true because the mode definition makes a set of executors definitions available, but only one is chosen in config.
Thu, Aug 6, 12:59 AM
sandyryza added inline comments to D4103: implement configured logger, executor.
Thu, Aug 6, 12:52 AM
sandyryza updated the diff for D4113: [docs] dagit overview.

up

Thu, Aug 6, 12:42 AM
sandyryza added a comment to D4113: [docs] dagit overview.

@yuhan - great point, I added in the schedule page.

Thu, Aug 6, 12:38 AM
sandyryza accepted D4109: Update language around py38 in docs.

👍

Thu, Aug 6, 12:34 AM

Wed, Aug 5

sandyryza requested review of D4113: [docs] dagit overview.
Wed, Aug 5, 9:35 PM
sandyryza updated the diff for D4106: Update CHANGES.md about config removal.

up

Wed, Aug 5, 6:40 PM
sandyryza updated the diff for D4036: migration guide.

up

Wed, Aug 5, 6:17 PM
sandyryza requested review of D4106: Update CHANGES.md about config removal.
Wed, Aug 5, 6:14 PM
sandyryza updated the diff for D4036: migration guide.

up

Wed, Aug 5, 5:58 PM
sandyryza updated the diff for D4054: experimental warning.

up

Wed, Aug 5, 4:47 PM
sandyryza updated the diff for D4087: nicer error on failure to load repository in external process.

up

Wed, Aug 5, 4:22 PM
sandyryza updated the diff for D4054: experimental warning.

up

Wed, Aug 5, 3:34 PM
sandyryza updated the diff for D4087: nicer error on failure to load repository in external process.

up

Wed, Aug 5, 3:33 PM
sandyryza updated the diff for D4054: experimental warning.

up

Wed, Aug 5, 1:29 AM
sandyryza added inline comments to D4022: refactor configured implementation + more docs.
Wed, Aug 5, 12:19 AM

Tue, Aug 4

sandyryza updated the diff for D4054: experimental warning.

up

Tue, Aug 4, 11:56 PM
sandyryza committed R1:5a180928c2a3: explain what devs must do after editing api.proto (authored by sandyryza).
explain what devs must do after editing api.proto
Tue, Aug 4, 11:54 PM
sandyryza closed D4095: explain what devs must do after editing api.proto.
Tue, Aug 4, 11:54 PM
sandyryza committed R1:706b25b5eaf5: remove deprecated "config" args that have been replaced with "config_schema" (authored by sandyryza).
remove deprecated "config" args that have been replaced with "config_schema"
Tue, Aug 4, 6:49 PM
sandyryza closed D4055: remove deprecated "config" args that have been replaced with "config_schema".
Tue, Aug 4, 6:49 PM
sandyryza requested review of D4087: nicer error on failure to load repository in external process.
Tue, Aug 4, 6:37 PM
sandyryza updated the diff for D4054: experimental warning.

up

Tue, Aug 4, 6:37 PM
sandyryza requested review of D4095: explain what devs must do after editing api.proto.
Tue, Aug 4, 6:16 PM
sandyryza updated the diff for D4055: remove deprecated "config" args that have been replaced with "config_schema".

up

Tue, Aug 4, 6:00 PM
sandyryza updated the diff for D4054: experimental warning.

up

Tue, Aug 4, 5:46 PM
sandyryza requested review of D4054: experimental warning.

@schrockn thanks for the accept, but I had noticed some trickiness around classes and wanted to see if you had an opinion on the approach. Comment repeated here for your convenience:

Tue, Aug 4, 4:33 PM
sandyryza accepted D4024: implement configurable (pure) solids.

LGTM! Put an underscore before configured_config_mapping_fn to be consistent with previous change?

Tue, Aug 4, 2:47 PM
sandyryza committed R1:374eb5e89e47: pull back dagster_type arg to asset materializations (authored by sandyryza).
pull back dagster_type arg to asset materializations
Tue, Aug 4, 12:00 AM
sandyryza added a reverting change for D4023: modifying metadata with prefix: R1:374eb5e89e47: pull back dagster_type arg to asset materializations.
Tue, Aug 4, 12:00 AM
sandyryza added a reverting change for D3985: Adding type info to materializations as metadata: R1:374eb5e89e47: pull back dagster_type arg to asset materializations.
Tue, Aug 4, 12:00 AM
sandyryza added a reverting change for R1:36037b9a56aa: Adding type info to materializations as metadata: R1:374eb5e89e47: pull back dagster_type arg to asset materializations.
Tue, Aug 4, 12:00 AM
sandyryza added a reverting change for R1:011049bc875a: modifying metadata with prefix: R1:374eb5e89e47: pull back dagster_type arg to asset materializations.
Tue, Aug 4, 12:00 AM
sandyryza closed D4056: pull back dagster_type arg to asset materializations.
Tue, Aug 4, 12:00 AM

Mon, Aug 3

sandyryza accepted D4075: Undeprecate create_dagster_pandas_dataframe_type.
Mon, Aug 3, 11:31 PM
sandyryza accepted D4022: refactor configured implementation + more docs.

Had one small comment. Otherwise this LGTM!

Mon, Aug 3, 10:54 PM
sandyryza requested review of D4056: pull back dagster_type arg to asset materializations.
Mon, Aug 3, 3:51 PM
sandyryza updated the diff for D4055: remove deprecated "config" args that have been replaced with "config_schema".

up

Mon, Aug 3, 3:49 PM
sandyryza updated the summary of D4055: remove deprecated "config" args that have been replaced with "config_schema".
Mon, Aug 3, 3:48 PM
sandyryza retitled D4055: remove deprecated "config" args that have been replaced with "config_schema" from remove "config" args that have been replaced with "config_schema" to remove deprecated "config" args that have been replaced with "config_schema".
Mon, Aug 3, 3:47 PM
sandyryza updated subscribers of D4022: refactor configured implementation + more docs.
Mon, Aug 3, 3:44 PM

Fri, Jul 31

sandyryza updated the diff for D4055: remove deprecated "config" args that have been replaced with "config_schema".

up

Fri, Jul 31, 11:18 PM
sandyryza updated the diff for D4054: experimental warning.

up

Fri, Jul 31, 11:14 PM
sandyryza added a comment to D4054: experimental warning.

The class situation is actually trickier. Decorating __init__ just gives a warning saying "__init__ is experimental". Ideally we'd include the class name in the warning. A few potential solutions:

  • A decorator for the class. I implemented this in the latest version. A hitch is that there's no built-in equivalent of functools.wraps for classes, so would need to implement our own if we want the class to behave exactly like the undecorated version. Here's one way to do this: https://stackoverflow.com/a/6394966
  • Expect the user to explicitly name the class when decorating __init__.
  • Expect the user to call experimental_class_warning from their __init__.
Fri, Jul 31, 11:13 PM
sandyryza requested review of D4055: remove deprecated "config" args that have been replaced with "config_schema".
Fri, Jul 31, 9:57 PM
sandyryza updated the diff for D4054: experimental warning.

up

Fri, Jul 31, 9:39 PM
sandyryza added a comment to D4054: experimental warning.

The warnings library already has pretty robust functionality around letting users mute warnings: https://docs.python.org/3/library/warnings.html

Fri, Jul 31, 9:30 PM
sandyryza accepted D4040: added simplified lakehouse example.

I noticed a final few small issues. Once those are fixed, this looks 100% good to go!

Fri, Jul 31, 9:10 PM
sandyryza added a comment to D4054: experimental warning.

for classes would be annotate the init method?

Fri, Jul 31, 8:58 PM
sandyryza added a comment to D4054: experimental warning.

What about experimental arguments to existing APIs?

Fri, Jul 31, 6:58 PM
sandyryza updated the diff for D4054: experimental warning.

up

Fri, Jul 31, 6:58 PM
sandyryza added a reverting change for D4023: modifying metadata with prefix: D4056: pull back dagster_type arg to asset materializations.
Fri, Jul 31, 6:09 PM
sandyryza added a reverting change for D3985: Adding type info to materializations as metadata: D4056: pull back dagster_type arg to asset materializations.
Fri, Jul 31, 6:09 PM
sandyryza added a reverting change for R1:36037b9a56aa: Adding type info to materializations as metadata: D4056: pull back dagster_type arg to asset materializations.
Fri, Jul 31, 6:09 PM
sandyryza added a reverting change for R1:011049bc875a: modifying metadata with prefix: D4056: pull back dagster_type arg to asset materializations.
Fri, Jul 31, 6:09 PM
sandyryza requested review of D4054: experimental warning.
Fri, Jul 31, 5:50 PM

Thu, Jul 30

sandyryza added a comment to D3981: [2] run-scoped file manager resources.

This LGTM! Maybe we should just replace the non-run-scoped one with this. Thoughts @max?

Thu, Jul 30, 4:14 PM

Wed, Jul 29

sandyryza updated the diff for D4036: migration guide.

up

Wed, Jul 29, 10:34 PM
sandyryza published D4036: migration guide for review.
Wed, Jul 29, 9:39 PM

Mon, Jul 27

sandyryza accepted D3953: gcs and azure file manager resources.
Mon, Jul 27, 9:00 PM

Fri, Jul 24

sandyryza added inline comments to D3990: added utility functions to load config from yaml files, strings, and package resources..
Fri, Jul 24, 9:29 PM
sandyryza added inline comments to D3985: Adding type info to materializations as metadata.
Fri, Jul 24, 9:01 PM
sandyryza committed R1:09e2f2c63d7a: [docs] Cover "configured" in configuration overview (authored by sandyryza).
[docs] Cover "configured" in configuration overview
Fri, Jul 24, 8:58 PM
sandyryza closed D4008: [docs] Cover "configured" in configuration overview.
Fri, Jul 24, 8:58 PM
sandyryza committed R1:0399a89ec1e0: module doc for simple lakehouse example (authored by sandyryza).
module doc for simple lakehouse example
Fri, Jul 24, 8:57 PM
sandyryza closed D4005: module doc for simple lakehouse example.
Fri, Jul 24, 8:57 PM
sandyryza requested review of D4005: module doc for simple lakehouse example.
Fri, Jul 24, 8:18 PM
sandyryza requested review of D4008: [docs] Cover "configured" in configuration overview.
Fri, Jul 24, 8:14 PM
sandyryza requested review of D3848: lakehouse generate asset materializations.
Fri, Jul 24, 5:27 PM
sandyryza accepted D3961: Added verification that fxn wrapped by resource has the correct number of positional args..

LGTM!

Fri, Jul 24, 5:23 PM
sandyryza requested changes to D3961: Added verification that fxn wrapped by resource has the correct number of positional args..
Fri, Jul 24, 3:57 PM
sandyryza committed R1:1065375dd9eb: use configured in simple_lakehouse example (authored by sandyryza).
use configured in simple_lakehouse example
Fri, Jul 24, 3:54 PM
sandyryza closed D3892: use configured in simple_lakehouse example.
Fri, Jul 24, 3:54 PM
sandyryza added inline comments to D3892: use configured in simple_lakehouse example.
Fri, Jul 24, 3:54 PM
sandyryza accepted D3985: Adding type info to materializations as metadata.
Fri, Jul 24, 3:51 PM
sandyryza added inline comments to D3990: added utility functions to load config from yaml files, strings, and package resources..
Fri, Jul 24, 3:37 PM
sandyryza added inline comments to D3985: Adding type info to materializations as metadata.
Fri, Jul 24, 3:33 PM
sandyryza added inline comments to D3961: Added verification that fxn wrapped by resource has the correct number of positional args..
Fri, Jul 24, 3:24 PM
sandyryza added inline comments to D3985: Adding type info to materializations as metadata.
Fri, Jul 24, 3:19 PM
sandyryza committed R1:20a9fde160e3: custom linter rule to catch print statements (authored by sandyryza).
custom linter rule to catch print statements
Fri, Jul 24, 12:22 AM
sandyryza closed D3905: custom linter rule to catch print statements.
Fri, Jul 24, 12:22 AM
sandyryza requested changes to D3966: be explicit that resources can't be configured twice.
Fri, Jul 24, 12:17 AM
sandyryza added a comment to D3966: be explicit that resources can't be configured twice.

Would it make sense to raise an error when configured is called on any resource that has an empty config schema? From the perspective of configured, a resource with all its configuration curried in is identical to a resource that had no configuration in the first place.

Fri, Jul 24, 12:17 AM
sandyryza added a comment to D3998: first pass fix resource config preprocessing issue.

This LGTM! Should we unmark those xfail test?

Fri, Jul 24, 12:12 AM

Thu, Jul 23

sandyryza accepted D3994: [resource] Fixed arguments for a bunch of resource definitions that were not using any positional arguments.
Thu, Jul 23, 10:49 PM
sandyryza requested review of D3892: use configured in simple_lakehouse example.
Thu, Jul 23, 8:16 PM
sandyryza accepted D3993: [docs] Remove old version links - 0.7.8 and older.
Thu, Jul 23, 7:48 PM