Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[pythonic resources] Add support for Pythonic resource classes to @configured #18079

Merged
merged 4 commits into from
Nov 27, 2023
Merged

[pythonic resources] Add support for Pythonic resource classes to @configured #18079

merged 4 commits into from
Nov 27, 2023

Conversation

benpankow
Copy link
Member

@benpankow benpankow commented Nov 16, 2023 •
edited
Loading

Summary

Responds to functionality request in #18073.

#14624 did most of the heavy lifting for this change, but erroneously updated the apidoc for @configured to indicate that it supported Pythonic resources while not actually doing so.

This PR makes a few lightweight changes to the internals of @configured to properly handle the differences between the Pythonic resource & config classes.

The following use-cases are now possible:

class MyResource(ConfigurableResource):
  my_string: str

# no args
@configured(MyResource)
def my_resource_noargs(_):
  return MyResource(my_string="foo")

# args, as old-style config dict
@configured(MyResource, {"an_int": int}):
def my_resource_from_int(config):
  return MyResource(my_string=str(config["an_int"]))

# args, as Pythonic config
class FromFloatConfig(Config):
  a_float: float

@configured(MyResource)
def my_resource_from_float(config: FromFloatConfig):
  return MyResource(my_string=str(config.a_float))

defs = Definitions(
  ...
  resources = {
    "noargs": my_resource_noargs,
    "from_int": my_resource_from_int.configured({"an_int": 5})
    "from_float": my_resource_from_float.configured({"a_float": 2.5})
  }
)

I think this is nice to support as a migration pattern to plug in new-style resources & the future support burden is pretty low.

That being said, it's worth highlighting there's a fairly native way to do this already (but not documented):

class MyResource(ConfigurableResource):
  my_string: str

class MyResourceNoargs(ConfigurableResource):
    def create_resource(self, context: InitResourceContext) -> Any:
        return MyResource(my_string="foo")

class MyResourceFromInt(ConfigurableResource):
    an_int: int

    def create_resource(self, context: InitResourceContext) -> Any:
        return MyResource(my_string=str(self.an_int))

defs = Definitions(
  ...
  resources = {
    "noargs": MyResourceNoargs(),
    "from_int": MyResourceFromInt(an_int=5)
  }
)

Test Plan

New unit test suite.

@benpankow
Copy link
Member Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@benpankow benpankow force-pushed the benpankow/configured-support-pythonic-resources branch from 8d7cd25 to 77d2bfe Compare November 27, 2023 14:31
@github-actions GitHub Actions
Copy link

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-564fhvhh6-elementl.vercel.app
https://benpankow-configured-support-pythonic-resources.core-storybook.dagster-docs.io

Built with commit 77d2bfe.
This pull request is being automatically deployed with vercel-action

@github-actions GitHub Actions
Copy link

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-jjx7vcwjd-elementl.vercel.app
https://benpankow-configured-support-pythonic-resources.dagster.dagster-docs.io

Direct link to changed pages:

@benpankow benpankow merged commit 368fe07 into master Nov 27, 2023
2 of 3 checks passed
@benpankow benpankow deleted the benpankow/configured-support-pythonic-resources branch November 27, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schrockn schrockn schrockn approved these changes

@alangenfeld alangenfeld Awaiting requested review from alangenfeld

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benpankow @schrockn