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

conda list and clean commands #1803

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

madhur-ob
Copy link
Collaborator

@madhur-ob madhur-ob commented Apr 16, 2024

TODOs:

  • How to associate step names with environment IDs?
  • Where are dependency lock files located? Since we need to delete those as well..

@iamsgarg-ob
Copy link

LGTM.. just not sure how easy or hard it is to add tests.

@romain-intel
Copy link
Contributor

haven't looked yet in detail but I think we should name it environment to keep consistency with the extension.

@savingoyal
Copy link
Collaborator

@romain-intel, this is more of a hidden utility to clean out the underlying conda environments than providing a top-level environment manipulation command. there is a good reason that we may want to have environment as yet another command in the near future.

@romain-intel
Copy link
Contributor

I mean, it would be exposed to the user and there is similarity with this: https://github.com/Netflix/metaflow-nflx-extensions/blob/main/metaflow_extensions/netflix_ext/plugins/environment_cli.py so I wanted to try to minimize (more) divergence. There is also an argument for doing this in metaflow environment as opposed to myflow.py --environment=conda environment but that may be a larger conversation. If this is just something hidden, we should hide it better or provide a utility. Also, we could start by providing these commands and then expand to others. I just feel a bit uncomfortable providing a user-visible "hidden utility" like this I guess.

@madhur-ob
Copy link
Collaborator Author

I remember asking @savingoyal if we wanna add this at the metaflow conda level or perhaps python flow.py conda level.
The latter was probably chosen because we kind of operate on a per flow basis i.e. conda environments for each step etc. and we want the ability to list them + clean them etc.

At the metaflow conda level, it would be a bit difficult to get the ctx.obj I guess -- which allows all that functionality. I could be wrong though.

@romain-intel
Copy link
Contributor

I remember asking @savingoyal if we wanna add this at the metaflow conda level or perhaps python flow.py conda level. The latter was probably chosen because we kind of operate on a per flow basis i.e. conda environments for each step etc. and we want the ability to list them + clean them etc.

At the metaflow conda level, it would be a bit difficult to get the ctx.obj I guess -- which allows all that functionality. I could be wrong though.

Totally agree that it doesn't necessarily have to be at the metaflow level. However, I would prefer it be named environment to be more compatible with the CLI in the extension (it's one or the other which makes more sense).

@saikonen saikonen self-requested a review April 30, 2024 16:06
metaflow/plugins/pypi/conda_cli.py Outdated Show resolved Hide resolved
Comment on lines 124 to 126
manifest_path = os.path.join(
ctx.obj.flow_datastore.datastore_root, ctx.obj.flow.name, MAGIC_FILE
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same problem as with list_envs. Not getting any environments when default datastore is not local.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, can you check again please

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is still bugging out, though cause is my messy conda manifest:

TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType

@conda.command(name="list", help="List conda environments for a flow.")
@click.option(
"-a",
"--show-all",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting an error with --show-all with my local manifest:

    print("{:<18} {:<67} {:<8} {:<12} {:<12}".format(row))
TypeError: unsupported format string passed to NoneType.__format__

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you help me with debugging this...
to start with, what is the value of row?

also, what are the values of item, envs_data etc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shared my conda manifest for testing purposes. The failing row for displaying is this:

['6bf4a51d2573ea3', None, '', 'osx-32', 'conda']

which most likely is leftovers from my development with the PyPI deco.

MAGIC_FILE,
)
if not os.path.exists(manifest_path):
print(f"No conda environments for the flow: {ctx.obj.flow.name}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f-string borking python 3.5 tests. Note the other occurrences as well.

@conda.command(name="list", help="List conda environments for a flow.")
@click.option(
"-a",
"--show-all",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shared my conda manifest for testing purposes. The failing row for displaying is this:

['6bf4a51d2573ea3', None, '', 'osx-32', 'conda']

which most likely is leftovers from my development with the PyPI deco.

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.

None yet

5 participants