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

feature: Conda env extension hooks #1902

Merged
merged 5 commits into from
Jul 1, 2024

Conversation

saikonen
Copy link
Collaborator

@saikonen saikonen commented Jun 26, 2024

We need some additional extension capabilities to be able to implement #1796 as an extension, these being:

  • add 'docker' to list of supported environments for @conda and @pypi
  • ability to fallback to the core Conda environment initialization for a subset of steps only.

Note: the changes to conda_environment.py would at the moment result in any plugin relying on the new only_steps= to break with the bleeding edge nflx-extension conda implementation


# 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"]
Copy link
Collaborator

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

Copy link
Collaborator Author

@saikonen saikonen Jun 27, 2024

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

Copy link
Collaborator Author

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.

@savingoyal savingoyal merged commit 728668d into master Jul 1, 2024
26 checks passed
@savingoyal savingoyal deleted the feature/conda-env-extension-hooks branch July 1, 2024 16:48
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

Successfully merging this pull request may close these issues.

2 participants