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

Context/Scope of vars not working when schema/mixin imported from module #1777

Open
diefans opened this issue Dec 9, 2024 · 8 comments
Open

Comments

@diefans
Copy link

diefans commented Dec 9, 2024

Bug Report

I am very confused, how the scope/context is applied resp. how _default_config is resolved.

My naive understanding was (before I moved the definitions to the module) that the scope/context is created at "init-time" and is somehow passed/"shining" from module level through the deeper schema tree... but now this issue confuses my a lot...

1. Minimal reproduce step (Required)

If I move schema/mixin/protocol definitions to a module a context var is not found: the working example is here

# INFO: this is in schemata.foo
protocol LoginProtocol:
    a_url: str
    b_url: str

mixin DefaultLoginMixin for LoginProtocol:
    # INFO: If no a_url, b_url is defined in the final schema, we apply the
    # value from _default_login
    a_url: str = a_url or _default_login.a_url
    b_url: str = b_url or _default_login.b_url

schema Config:
    mixin [
        DefaultLoginMixin
    ]

schema Stage:
    [...str]: Config
    _default_login: LoginProtocol
import schemata.foo

ci00 = foo.Stage {
    # FIXME: how is _default_login reaching the mixin, when defined in one script,
    # but not if the mixin is imported from a module?
    _default_login = foo.LoginProtocol {
        a_url = "https://default.a_url"
        b_url = "https://default.b_url"
    }
    provider_1 = foo.Config {}
    provider_2 = foo.Config {
        # INFO: override the default
        a_url = "https://custom.a_url"
        b_url = "https://custom.b_url"
    }
}

2. What did you expect to see? (Required)

ci00:
  provider_1:
    a_url: https://default.a_url
    b_url: https://default.b_url
  provider_2:
    a_url: https://custom.a_url
    b_url: https://custom.b_url

3. What did you see instead (Required)

EvaluationError
 --> /home/olli/code/bm/integration-config/schemata/foo.k:8:1
  |
8 |     a_url: str = a_url or _default_login.a_url
  |  invalid value 'UndefinedType' to load attribute 'a_url'
  |

4. What is your KCL components version? (Required)

❯ kcl --version
kcl version v0.10.8
@diefans
Copy link
Author

diefans commented Dec 10, 2024

Even if I define the Config schema in the same module, as where _default_config is created/instantiated/scoped, it still does not work. So this seems to defy the meaning of "mixin"...

import schemata.foo

schema Config:
    # FIXME: seems like resolution of _default_login is just looking into,
    # where the mixin is defined and not where it is applied/used/mixed-in
    mixin [
        foo.DefaultLoginMixin
    ]

schema Stage:
    [...str]: Config
    _default_login: foo.LoginProtocol

ci00 = Stage {
    # FIXME: how is _default_login reaching the mixin, when defined in one script,
    # but not if the mixin is imported from a module?
    _default_login = foo.LoginProtocol {
        a_url = "https://default.a_url"
        b_url = "https://default.b_url"
    }
    provider_1 = Config {}
    provider_2 = Config {
        # INFO: override the default
        a_url = "https://custom.a_url"
        b_url = "https://custom.b_url"
    }
}

@diefans
Copy link
Author

diefans commented Dec 20, 2024

@Peefy can you confirm, that my observation makes sense to you...? I also tried to find the place in the code, where I can start to fix it by myself... (resolving this would greatly improve our kcl code)

@Peefy
Copy link
Contributor

Peefy commented Dec 20, 2024

cc @zong-zhe @He1pa

@He1pa
Copy link
Contributor

He1pa commented Dec 30, 2024

It seems that you have some misunderstanding about the use of mixin and protocol

We have an example about Protocol and Mixin: https://www.kcl-lang.io/docs/next/reference/lang/tour#advanced

For your case, you can implement it with the following code

schema DefalutConfig:
    a_url: str
    b_url: str

protocol LoginProtocol:
    _default_login: DefalutConfig
    _custom_a_url?: str
    _custom_b_url?: str

mixin DefaultLoginMixin for LoginProtocol:
    a_url: str = _custom_a_url or _default_login.a_url
    b_url: str = _custom_b_url or _default_login.b_url

schema Config:
    mixin [
        DefaultLoginMixin
    ]
    _default_login: DefalutConfig
    _custom_a_url?: str
    _custom_b_url?: str

schema Stage:
    [...str]: Config
    _default_login: DefalutConfig

ci00 = Stage {
    _default_login = DefalutConfig {
            a_url = "https://default.a_url"
            b_url = "https://default.b_url"
        }
    provider_1 = Config {
        _default_login = _default_login
        
    }
    provider_2 = Config {
        _default_login = _default_login
        _custom_a_url = "https://custom_a_url.a_url"
        _custom_b_url = "https://custom_b_url.b_url"
    }
}

@diefans
Copy link
Author

diefans commented Dec 30, 2024

Thanks @He1pa for the example, and I can follow it, but (and I should have copied the working example into this ticket to outline my amazement)... I am still wondering if this is a bug or a feature, since it is working without explicitly introducing _default_login into the scope of Config {...}

protocol LoginProtocol:
    a_url: str
    b_url: str

mixin DefaultLoginMixin for LoginProtocol:
    # INFO: If no a_url, b_url is defined in the final schema, we apply the
    # value from _default_login
    a_url: str = a_url or _default_login.a_url
    b_url: str = b_url or _default_login.b_url

schema Config:
    mixin [
        DefaultLoginMixin
    ]

schema Stage:
    [...str]: Config
    _default_login: LoginProtocol

ci00 = Stage {
    # FIXME: how is _default_login reaching the mixin, when defined in one script,
    # but not if the mixin is imported from a module?
    _default_login = LoginProtocol {
        a_url = "https://default.a_url"
        b_url = "https://default.b_url"
    }
    provider_1 = Config {}
    provider_2 = Config {
        # INFO: override the default
        a_url = "https://custom.a_url"
        # the value comes from _default_login here
        # b_url = "https://custom.b_url"
    }
}
config = Config {
    # without a_url and b_url we get an error here
    a_url = "https://must.be.set.a_url"
    b_url = "https://must.be.set.b_url"
}

resulting in

ci00:
  provider_1:
    a_url: https://default.a_url
    b_url: https://default.b_url
  provider_2:
    a_url: https://custom.a_url
    b_url: https://default.b_url
config:
  a_url: https://must.be.set.a_url
  b_url: https://must.be.set.b_url

So the bug I was assuming I have found, was, that when moving the definition of DefaultLoginMixin to another module this is throwing the aforementioned error - it would be nice, that you give an explanation why this is working and if it is supposed to be in accordance to the spec.

My explanation that this works is, that Stage._default_login provides the context for the mixin attached to Config, since this is "embedded" in Stage and the mixin is just picking up the surrounding context. Defining Config outside of Stage requires to set a_url and b_url (which makes sense).

@He1pa
Copy link
Contributor

He1pa commented Jan 6, 2025

This is an undesigned situation. We never considered that Protocol appears as an attribute in attr. like

schema Stage:
    [...str]: Config
    _default_login: LoginProtocol

Protocol is some assembly logic of Mixin. And Mixin is used in the way of mixin[xxx]. Maybe some checks should be added to avoid this usage.

@diefans
Copy link
Author

diefans commented Jan 6, 2025

@He1pa this protocol feature is only a "byproduct" of my issue... sorry

The main topic/question is: why is the example working, when everything (mainly DefaultLoginMixin definition) is in one file/module and why is it not working when I put the DefaultLoginMixin definition into different module and import it from there?

I have created a repo for this issue, so you can just check it out and test easily the difference - since I have the impression, that my issue is not really understood. I have also removed the usage of protocol-as-a-schema: https://github.com/bettermarks/kcl-issue-1777

So the issue is about the import of the mixin and why does it break.

@He1pa
Copy link
Contributor

He1pa commented Jan 7, 2025

ok, I will check again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants