Modify KedroContext with frozen attributes instead of frozen class

[noklam]

Signed-off-by: Nok [email protected]

Description

Fix #1459. Initially we plan to freeze the class to prevent users changing the internal details. After some discussion, this can be too restrictive and we decide to only freeze existing attributes. Users are free to create their own as long as it doesn't conflict with the internal one.

Development notes

For Reviewer

@define(slots=False)  # Enable setting new attributes to `KedroContext`
class KedroContext:
    """``KedroContext`` is the base class which holds the configuration and
    Kedro's main functionality.
    """

    _package_name: str = field(init=True, on_setattr=frozen)
    project_path: Path = field(
        init=True, converter=_expand_full_path, on_setattr=frozen
    )
    config_loader: AbstractConfigLoader = field(init=True, on_setattr=frozen)
    _hook_manager: PluginManager = field(init=True, on_setattr=frozen)
    env: str | None = field(init=True, on_setattr=frozen)
    _extra_params: dict[str, Any] | None = field(
        init=True, default=None, converter=deepcopy, on_setattr=frozen
    )

Looking at the current interface, should we re-order the argument as it seems arbitary, (and) maybe make it keyword only.

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

• Read the contributing guidelines
• Signed off each commit with a Developer Certificate of Origin (DCO)
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the RELEASE.md file
• Added tests to cover my changes
• Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

[noklam]

@astrojuanlu I think this should work, however I am stuck with a testing issue. There are two problems

1. Mock doesn't work well with autospec, I've managed to find a workaround to automatically generate the spec.
2. https://github.com/quantumblacklabs/private-kedro/pull/975 - dynaconf (the library that we used automatically delete the attributes)

mock_cls = create_attrs_autospec(KedroContext)

If you inspect this object you should see that all attributes are there, yet if you look at the test it complains

Input: "Mock object has no attribute 'env'". The issue that I link seems to suggest that this is a known issue.

[noklam]

I manage to fix the test.

@Galileo-Galilei Any chance you can test with this branch?

[Galileo-Galilei]

I'll do it tonight

[merelcht]

Generally this looks good to me. In the description you asked:

Looking at the current interface, should we re-order the argument as it seems arbitary, (and) maybe make it keyword only.

I 100% agree this order looks arbitrary now, especially because there's a mix of "internal"/"private" arguments starting with _ and not. I think now would be the right moment to re-order, because it's a breaking change.

[Galileo-Galilei]

Hi @noklam and thank for adressing it! I haven't looked at the code in detail yet, but I have tested the behaviour and it works as expected:

• The attributes are indeed frozen, and I can add attributes (just with context.next_attr="value", not need to use a workaround with object.__setattr__)
• kedro-mlflow does work with this version 👍

I agree that it is likely time to reorder all arguments to make clearer which one are public.

However, I still have more generic concerns on whether all (or even any) attributes should be frozen. The after_context_hook is particularly useful to distribute the logic without relying on the template, which makes it much easier to update and maintain. For instance, we have a plugin at work which register a custom ConfigLoader (basically because we want to fetch the conf from another place, but we want to benefit from all the capabilities of kedro for resolving - related to #2481, but this is another story). If we cannot modify it through the hook, we will need to tell our users to update their templates on their which is basically a nightmare in maintenance.

I can also think of use cases in which I want to update the _extra_params dictionary, but since the underlying object is mutable, context._extra_params.update({"new_param":"value"}) works.

By order of preference:

1. Do not freeze anything
2. Freeze all attributes except ConfigLoader
3. Stay as is (but I am afraid that it will break some of our internal workflows at work, I need to check but I am not sure I could make it before the release)

[noklam]

I have decided to not freeze any attribute at the end. Choosing some attributes to freeze and not others feels arbitrary to me, let's just fallback to Python convention with _. On the other hand, we should still respect that this is something internal to Kedro, so this is a "do it at your own risk", but of course we should't break it easily without offering a valid alternative.

Additionally, I re-arrange the order of the argument. Noted that package_name will be removed soon so it's irrelevant.

[merelcht]

Thanks @Galileo-Galilei for testing and explaining your pain points. This is really insightful. I agree with the approach to not freeze anything in the end @noklam 👍

I was just wondering if you then still need all the tests for checking attributes?

[merelcht]

Is this still needed now we're not freezing anything?

[noklam]

I was just wondering if you then still need all the tests for checking attributes?

It is needed, and for this reason I keep one of the test. If you try to set slots=True (or remove this argument since it's default), you will see the test fail. The default for standard Python library is False

https://www.attrs.org/en/stable/api.html#attrs.define - The reason for this is slotted class is more efficient and make sense if it is used as "dataclass" (think of pydantic model that get convert to JSON etc), in this case it's irrelevant

[merelcht]

Thank you @noklam for revisiting this change, good thing we didn't release it the way it was 👍

[ankatiyar]

Thanks @noklam 😄

[Galileo-Galilei]

Thank you everyone and especially @noklam for this change, and sorry for not having been as available as I had wanted to to help for this!