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

Reconsider making the KedroContext a frozen attrs before 0.19 release #3214

Closed
Galileo-Galilei opened this issue Oct 22, 2023 · 2 comments
Closed
Labels
Issue: Feature Request New feature or improvement to existing feature

Comments

@Galileo-Galilei
Copy link
Contributor

Galileo-Galilei commented Oct 22, 2023

Description

The KedroContext has been made a frozen dataclass in the develop branch since May 2022 and the PR #1465.
I think it should be unfrozen to keep te current behaviour and enable users to modify the context.

The PR has triggered a lot of discussions (see in the issue), but it can be summarized as follows:

  1. if the class is frozen, it prevents users to modify the context with unintended side_effects, which has been made easier with the after_context_created
  2. if the class is not frozen, users can add attributes to the context, e.g. plugin configuration. The main advantage of this approach is that it makes the "interactive" workflow (i.e. within a notebook) consistent with the "command line" workflow (i.e. running with the kedro run command).

Basically, we (@antonymilne, @noklam and myself) were not absolutely sure of what the best decision was, but @noklam pointed out that we have a workaround to force setting the attribute of a frozen dataclass by using context.__setattr__("attribute_name", attribute_value) so we could have the benefits of both 1 & 2.

Context

I am currently preparing the migration of kedro-mlflow to kedro=0.19, and it turns out that the workaround identified back then does not work. I can try do dark magic by digging deeply in python internals and kind of redefining __setattr__ on the fly, but it looks very (very!) bad and I am not even sure I can make it work properly.

Unless we find a workaround, we are forced to choose between 1 & 2. My personal preference go to 2. for the following reasons:

  • Kedro has been open source for 4 years now, and I have never seen a bug due to a user involuntarily modifying the context, even during the last past year with the after_context_created hook. This obviously may happen, but we should acknowledge that the few users who modify the context do it "at their own risk" and know what they do.
  • I personally think (but I am biased as a plugin author 😅 ) that the consistency with the interactive workflow (not doing so will basically remove the mlflow configuration from context and make it hard to use in notebooks) and the simplified experience for plugin authors is worth letting them modifying the class.

Possible Implementation

Replace @frozen decorator by @define(slots=False). The @slots=False is needed to enable adding new attributes.

@frozen
class KedroContext:
"""``KedroContext`` is the base class which holds the configuration and
Kedro's main functionality.

Possible Alternatives

Keep it frozen.

@inigohidalgo
Copy link
Contributor

Thanks @Galileo-Galilei for this heads up. In the slack we were discussing a workaround to preserve the old os.environ into globals functionality, and the cleanest solution was context.config_loader._globals.update(os.environ) in after_context_created hook. This change would invalidate this solution.

This workaround will only be temporary for us and we shouldn't be using it by the time we get to 0.19, but modifying the context does seem to be a commonly-used escape hatch when trying to do things which don't exactly fit in the "kedro way"

@Galileo-Galilei
Copy link
Contributor Author

Close in favor of #1459.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Feature Request New feature or improvement to existing feature
Projects
Archived in project
Development

No branches or pull requests

2 participants