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

Should we remove KEDRO_ENV? #1842

Closed
5 tasks
antonymilne opened this issue Sep 8, 2022 · 6 comments
Closed
5 tasks

Should we remove KEDRO_ENV? #1842

antonymilne opened this issue Sep 8, 2022 · 6 comments
Labels
Issue: Feature Request New feature or improvement to existing feature

Comments

@antonymilne
Copy link
Contributor

antonymilne commented Sep 8, 2022

Originally noticed in #1355. I think this is probably a bit of tech debt that we should remove, but not without understanding what it was originally for and whether it's used by anyone.

@AntonyMilneQB said:

The KEDRO_ENV provides a mechanism for changing env and is picked up when the session is created:

env = env or os.getenv("KEDRO_ENV")

As far as I can see, the interactive workflow doesn't strictly need this since a user can do %reload_kedro --env=xxx to change environment [instead of doing kedro jupyter lab --env=xxx]. However, since removing this would be a breaking change and in theory affect the creation of sessions outside the interactive workflow also I left it as is. I think we should check with users whether and how it's used and then hopefully remove it in future.

@datajoely said:

Would just add I think the KEDRO_ENV env var has a lot of value outside of the interactive workflow.

... but I'm not sure what that value would be? The only place that KEDRO_ENV is used is KedroSession.create, which already has an env argument (which is what's used by kedro run --env etc.).

To do

  • understand why this was added in the first place and so what its use might be (my guess is it was just a way to inject env into a Jupyter session, and we now have a better way to do that using %reload_kedro)
  • check telemetry data to see if anyone ever specifies env in their kedro jupyter/ipython command
  • do some sort of user research to see whether anyone would mind if we removed it
  • decide on the basis of the above whether we should remove it (i.e. alter the kedro jupyter/ipython commands to remove env as an argument and remove KEDRO_ENV everywhere)
  • if we do remove it, decide whether such a removal should be considered de-facto non-breaking (if no one uses it)
@antonymilne antonymilne added the Issue: Feature Request New feature or improvement to existing feature label Sep 8, 2022
@datajoely
Copy link
Contributor

The env var approach is super common in orchestrator situations, removing this would be dangerous in my opinion. If anything I also want to take this further an ensure this variable is an auto-scoped global in templated config.

@antonymilne
Copy link
Contributor Author

@datajoely can you explain why this is common? What the commands that people run that rely on KEDRO_ENV as an environment variable? What does it achieve that the env flag doesn't?

If anything I also want to take this further an ensure this variable is an auto-scoped global in templated config.

Agreed, and this is very much on the agenda for the configuration overhaul. But I don't see how the KEDRO_ENV helps here. We don't currently set KEDRO_ENV = env; we set env = KEDRO_ENV.

@datajoely
Copy link
Contributor

Oh sorry I misunderstood what you meant, ignore me.

@SajidAlamQB SajidAlamQB self-assigned this Oct 11, 2022
@noklam
Copy link
Contributor

noklam commented Oct 18, 2022

Adding some relevant context from Discord, this maybe useful for some workaround - do we have a preferred solution if we remove KEDRO_ENV?

Also related to #1935

ToniMaroni — Today at 8:57 AM
Hi, is there a way to have the configurations in settings.py configurable based on the kedro environment. For example, I want the hooks for kedro-viz and kedro-neptune disabled in production environment. Thanks
datajoely — Today at 8:59 AM
Interesting usecase - best solution off the top of my head is to use the KEDRO_ENV environment variable and just include an if statement in settings.py

@SajidAlamQB SajidAlamQB removed their assignment Oct 18, 2022
@noklam noklam modified the milestones: Improve the Interactive Jupyter notebook workflow, Something Jupyter Apr 24, 2023
@astrojuanlu
Copy link
Member

I contend that it doesn't harm to have it and it can be useful in certain situations.

@astrojuanlu
Copy link
Member

We decided that it was not worth the churn, so we're closing this for now.

@astrojuanlu astrojuanlu closed this as not planned Won't fix, can't repro, duplicate, stale Dec 18, 2023
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
None yet
Development

No branches or pull requests

5 participants