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

Add new CLI commands #225

Merged
merged 2 commits into from
Oct 17, 2022
Merged

Add new CLI commands #225

merged 2 commits into from
Oct 17, 2022

Conversation

jrbourbeau
Copy link
Member

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jrbourbeau jrbourbeau merged commit effef97 into conda-forge:main Oct 17, 2022
@jrbourbeau jrbourbeau deleted the new-cli branch October 17, 2022 17:28
Comment on lines +21 to +24
# New CLI added in https://github.com/dask/distributed/pull/6735
- dask scheduler = distributed.cli.dask_scheduler:main
- dask ssh = distributed.cli.dask_ssh:main
- dask worker = distributed.cli.dask_worker:main
Copy link
Member

@jakirkham jakirkham Oct 20, 2022

Choose a reason for hiding this comment

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

Idk if doing this with a space works as expected. What happens here is a binary is created that has a space in its name. For example...

$ which "dask scheduler"
/Users/jkirkham/miniforge/envs/dask/bin/dask scheduler

Though this isn't the same as...

$ which dask
/Users/jkirkham/miniforge/envs/dask/bin/dask

...or...

$ which dask scheduler
/Users/jkirkham/miniforge/envs/dask/bin/dask
scheduler not found

Should add dask scheduler with a space doesn't seem to work correctly either...

dask scheduler --help
Usage: dask [OPTIONS] COMMAND [ARGS]...
Try 'dask -h' for help.

Error: No such command 'scheduler'.

So there is still some work needed here (Idk what atm).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, shoot -- thanks for looking into this @jakirkham. It this a bit strange as this change appeared to fix related failing CI builds over in distributed. Regardless, I'm able to reproduce dask scheduler not working when I install the latest dask builds in a fresh conda environment:

$ mamba create -n test python=3.10 dask=2022.10.0
$ dask scheduler
Usage: dask [OPTIONS] COMMAND [ARGS]...
Try 'dask -h' for help.

Error: No such command 'scheduler'.

A couple of additional observations:

  • This appears to be a conda-specific thing. When I use pip to install a the latest dask + distributed in a fresh environment, dask scheduler exists and works as expected. This makes me think that all the entry_points / console_scripts logic is set up correctly.
  • I intentionally added tests for dask scheduler / dask worker / dask ssh in this PR that passed in CI. That makes this extra confusing.

@jakirkham do you have any thoughts on how we might be able to further debug here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The relevant entrypoint in distributed is defined here. Note that they're not console_scripts entrypoints, but are instead using the new dask_cli entrypoint. In this section of the conda-forge docs, it says:

Only console_scripts entry points have to be listed in meta.yaml. Other entry points do not conflict with noarch and therefore do not require extra treatment.

Based on that, I agree we don't need to include entries for these entrypoints in the recipe here. We included the changes in this PR and over in the dask feedstock at the same time. Based on these docs, maybe only the dask feedstock changes were needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying that out in #226

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this more, maybe we should try dropping the entrypoint from dask as well. Perhaps that fixes the issue?

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.

3 participants