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.

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

Missing config default_value in config_fn on composite_solid #1608

Closed
kinghuang opened this issue Jul 25, 2019 · 2 comments

Comments

@kinghuang
Copy link

commented Jul 25, 2019

I have a composite solid with an optional config named prefix, which is used in config_fn as the value for an inner solid's config. Since the config is optional, I expect the default value to be used if I don't specify a value in an environment. But, I get a KeyError on prefix instead (i.e., the config dict passed in config_fn doesn't have the prefix key at all).

Here's the composite solid and environment.

@composite_solid(
    config_fn=lambda _, cfg: {'prefix_value': {'config': {'prefix': cfg['prefix']}}},
    config={'prefix': Field(str, is_optional=True, default_value='_id_')}
)
def prefix_id(id):
    return prefix_value(id)
solids:
  prefix_id:
    config: {}
    inputs:
      id:
        value: "12345"

The empty config is due to #1607.

Here is the exception.

Exception occurred during execution of user config mapping function <lambda> defined by solid prefix_id from definition prefix_id at path root:solids:prefix_id:
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/dagster/core/types/evaluator/evaluation.py", line 252, in _evaluate_composite_solid_config
    frozendict(context.config_value.get('config')),
  File "/project/spendanalytics/nlp/normalize.py", line 17, in <lambda>
    config_fn=lambda _, cfg: {'prefix_value': {'config': {'prefix': cfg['prefix']}}},
KeyError: 'prefix'

Specifying a value for the prefix config works, but rather defeats the default value. I expect the config passed to config_fn to have a prefix key with the value _id_ from the composite's declared config.

solids:
  prefix_id:
    config:
      prefix: _foo_
    inputs:
      id:
        value: "12345"
@kinghuang kinghuang changed the title Cannot use config default_value in config_fn Missing config default_value in config_fn on composite_solid Jul 25, 2019
@mgasner mgasner added this to the 0.5.5 milestone Jul 25, 2019
@natekupp natekupp added bug core labels Jul 26, 2019
@natekupp

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Hey King Chung, thanks for reporting! This is indeed a real issue with the way we are currently applying default values from Field definitions.

Unfortunately I think it's going to take us a bit longer to fix, because the config traversal system needs a more holistic rework to support fixing defaults application. I added a note discussing the next steps on that front here: #692 (comment)

In the interim, I saw @mgasner was discussing presets with you on Slack, let us know if that is a good fit for your problem. I'll also follow up directly on Slack.

@helloworld helloworld self-assigned this Jul 29, 2019
helloworld added a commit that referenced this issue Aug 1, 2019
Summary:
See #1608

Also included small utility function `print_schema()` to make it easier to debug config_types

Example:

```
{
  solids: {
    print_value?: {
      outputs?: {
        result?: {
          |–json: {
          |  path: [Path]
          |}
          |–pickle: {
          |  path: [Path]
          |}
        }
      }
    } default={}
    prefix_id: {
      inputs: {
        id: {
          |–value: [ANY]
          |–json: {
          |  path: [Path]
          |}
          |–pickle: {
          |  path: [Path]
          |}
        }
      }
      outputs?: {
        result?: {
          |–json: {
          |  path: [Path]
          |}
          |–pickle: {
          |  path: [Path]
          |}
        }
      }
      config?: {
        prefix?: [String]  default=_id_
      } default={'prefix': '_id_'}
    }
  }
  expectations?: {
    evaluate?: [Bool]  default=True
  } default={'evaluate': True}
  storage?: {
    |–in_memory?: {
    |
    |} default={}
    |–filesystem?: {
    |  config?: {
    |    base_dir?: [String]
    |  } default={}
    |} default={'config': {}}
  }
  execution?: {

  } default={}
  loggers?: {
    console?: {
      config?: {
        log_level?: [String]  default=INFO
        name?: [String]  default=dagster
      } default={'log_level': 'INFO', 'name': 'dagster'}
    }
  } default={}
  resources?: {

  } default={}
}
```

Test Plan: unit

Reviewers: #ft, natekupp

Reviewed By: #ft, natekupp

Subscribers: natekupp, alangenfeld

Differential Revision: https://dagster.phacility.com/D753
@helloworld

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@helloworld helloworld closed this Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.