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

Investigate "default dodginess" #1990

Open
schrockn opened this issue Dec 14, 2019 · 5 comments
Open

Investigate "default dodginess" #1990

schrockn opened this issue Dec 14, 2019 · 5 comments
Assignees
Labels
tech-debt Problems with current implementation of a system/feature test

Comments

@schrockn
Copy link
Member

Found a comment that indicates a bug in test_system_config.py so creating an issue to track it down.

@schrockn schrockn self-assigned this Dec 14, 2019
@schrockn
Copy link
Member Author

Grep the file for the issue number

@mgasner mgasner added the test label Dec 16, 2019
@natekupp
Copy link
Contributor

There's still a comment for this issue in the codebase, but looks like the test is enabled—is this still an issue?

@mgasner mgasner added the core label May 27, 2020
@schrockn
Copy link
Member Author

Yes it's still an issue:

The config system claims that:

    @solid(
        name='int_config_solid',
        config=Field(Int, is_required=False),
        input_defs=[],
        output_defs=[],
    )
    def int_config_solid(_):
        return None

Has provided a default

@schrockn
Copy link
Member Author

assert int_solid_field.default_provided

@natekupp natekupp added this to the 0.9.0 (planned) milestone Jun 1, 2020
@natekupp natekupp modified the milestones: 0.9.0, 0.9.x Sep 16, 2020
@natekupp natekupp modified the milestones: 0.9.x, 0.10.0 Nov 10, 2020
@alangenfeld alangenfeld added the tech-debt Problems with current implementation of a system/feature label Mar 30, 2021
@sryza sryza removed this from the 0.10.0 milestone Apr 1, 2021
@sryza
Copy link
Contributor

sryza commented May 2, 2022

Is there a user-facing consequence to this?

I took a brief look at this, and as far as I can tell at least some part of the internals believe that this is the expected behavior - or at least we have code that explicitly tries to make it the case. I.e. the code snippets below go out of their way to make it the case that a Shape config field whose interior fields are all optional gets a default value of {}.

If I don't hear back, I'll post a PR that removes the comment and then close this issue.

Relevant snippets:

        if is_required is None:
            is_optional = has_implicit_default(self.config_type) or self.default_provided
            is_required = not is_optional

            # on implicitly optional - set the default value
            # by resolving the defaults of the type
            if is_optional and not self.default_provided:
                evr = resolve_defaults(self.config_type, None)
                if not evr.success:
                    raise DagsterInvalidConfigError(
                        "Unable to resolve implicit default_value for Field.",
                        evr.errors,
                        None,
                    )
                self._default_value = evr.value

def has_implicit_default(config_type):
    if config_type.kind == ConfigTypeKind.NONEABLE:
        return True

    return all_optional_type(config_type)
def all_optional_type(config_type: ConfigType) -> bool:
    check.inst_param(config_type, "config_type", ConfigType)

    if ConfigTypeKind.is_shape(config_type.kind):
        for field in config_type.fields.values():  # type: ignore
            if field.is_required:
                return False
        return True

    if ConfigTypeKind.is_selector(config_type.kind):
        if len(config_type.fields) == 1:  # type: ignore
            for field in config_type.fields.values():  # type: ignore
                if field.is_required:
                    return False
            return True

    return False

@yuhan yuhan removed the core label Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Problems with current implementation of a system/feature test
Projects
None yet
Development

No branches or pull requests

6 participants