-
Notifications
You must be signed in to change notification settings - Fork 765
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
feature: Conda env extension hooks #1902
Conversation
metaflow/metaflow_config.py
Outdated
|
||
# To placate people who don't want to see a shred of conda in UX, we symlink | ||
# --environment=pypi to --environment=conda | ||
return ["conda", "pypi"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would hardcoding docker
for now or relying on a property in docker_environment
to ensure compatibility work better? Managing it through config seems a bit anti-pattern-ish for the long-term since it is not a user exposed config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought on this for a bit and it felt a bit more natural to implement a way for the extensions to extend a supported list of environments, rather than them implementing a method for the decorator to call.
so the alternative approach here would be to implement something like the following, but only in environments that we support pypi/conda on?
# Environment
def _is_supported_deps_deco(self, deco_name):
return deco_name in ["conda", "pypi"]
and in the decorators env validation
# Decorator
try:
if not environment._is_supported_deps_deco(self.name):
raise InvalidEnvironmentException("...")
except Exception:
# env does not even implement the method for checking
echo("environment does not support pypi/conda")
raise
The main drawback with this approach is that we are now missing a list of environments that do support conda/pypi, so we can't display a helpful list to the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with the hardcoded option for the time being. Left a TODO regarding future improvements for extending the supported venv list.
We need some additional extension capabilities to be able to implement #1796 as an extension, these being:
@conda
and@pypi
Note: the changes to
conda_environment.py
would at the moment result in any plugin relying on the newonly_steps=
to break with the bleeding edge nflx-extension conda implementation