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 alchemiscale to conda-forge #23919

Merged
merged 9 commits into from
Oct 3, 2023

Conversation

mikemhenry
Copy link
Contributor

@mikemhenry mikemhenry commented Sep 6, 2023

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
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 (recipes/alchemiscale) and found it was in an excellent condition.

@mikemhenry
Copy link
Contributor Author

@dotsdl @hmacdope

GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.

@mikemhenry mikemhenry mentioned this pull request Sep 6, 2023
10 tasks
@hmacdope
Copy link
Contributor

hmacdope commented Sep 6, 2023

I am willing to be a maintainer :)

@mikemhenry
Copy link
Contributor Author

Thanks! 💤

@dotsdl
Copy link
Member

dotsdl commented Sep 8, 2023

I am willing to be a maintainer! Thanks @mikemhenry!

@mikemhenry
Copy link
Contributor Author

@conda-forge/help-python, ready for review!


build:
number: 0
skip: true # [win]
Copy link
Member

Choose a reason for hiding this comment

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

B/c the outputs are noarch this will be installable on Windows regardless of this skip. What is the reason for it? If it is just a missing dependency, then it is fine to remove it and let the solver deal with it. A failure in that case is not a blocker for merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do! I've gotten advice both ways and for noarch I also like to have the solver deal with it.

@mikemhenry mikemhenry requested a review from ocefpaf September 9, 2023 22:46
- pip check

about:
home: https://github.com/simplejson/simplejson
Copy link
Member

Choose a reason for hiding this comment

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

Ping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that is embarrassing 🙈

- numba
- pymbar >=3.0.6,<4
- async-lru
- nest-asyncio
Copy link
Member

Choose a reason for hiding this comment

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

Is this package alchemiscale-compute with this extra dependency? Is so you should avoid a dual build and depend on alchemiscale-compute here instead.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a perfect world, I would have some sort of alchecmiscale-base or have one depend on other other as you propose, but we are still moving very quickly on this development wise and in an iteration or two, the compute and client will be much more independent. So I figured once the dust settles on that I would re-work the dependencies.

Thank you for the suggestion!

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit busy at the moment with some personal things to resolve. Can you request a new reviewer. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Yea I don't get the extra overhead right now if this is a single source with different deps 🤔 Won't it be easier to address that in the feedstock once there and make the review simpler / quicker?

@mikemhenry
Copy link
Contributor Author

@ocefpaf Ready for another review 😄

@mikemhenry mikemhenry requested a review from ocefpaf September 12, 2023 14:14
@mikemhenry
Copy link
Contributor Author

@conda-forge/help-python, ready for review!

@mikemhenry
Copy link
Contributor Author

@conda-forge-admin, please ping team

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

@mikemhenry
Copy link
Contributor Author

mikemhenry commented Sep 14, 2023

Need a new reviewer on this PR (See #23919 (comment))

Thanks!

- requests
- click
- httpx
- pydantic<2.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- pydantic<2.0
- pydantic<2.0a0

- dask
- distributed
- numba
- pymbar >=3.0.6,<4
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- pymbar >=3.0.6,<4
- pymbar >=3.0.6,<4.0a0

@@ -0,0 +1 @@
${PYTHON} -m pip install . --no-deps -vv
Copy link
Member

Choose a reason for hiding this comment

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

I think you need some more flags here due to this bug.

@jaimergp
Copy link
Member

The CI is not doing any building 🤔 Does this work locally?

@mikemhenry
Copy link
Contributor Author

@jaimergp thanks for the feedback! It was working locally when I first submitted the package, I will try again to double-check.

Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
@mikemhenry
Copy link
Contributor Author

@jaimergp Okay ready for another pass, this is my first multi output feedstock I've submitted, so I appreciate the feedback!

@mikemhenry
Copy link
Contributor Author

Oh and should I be adding PIP_NO_INDEX=True PIP_NO_DEPENDENCIES=True PIP_NO_BUILD_ISOLATION=False PIP_IGNORE_INSTALLED=True PYTHONDONTWRITEBYTECODE=True to every multi output feedstock?

@mikemhenry mikemhenry requested a review from jaimergp September 20, 2023 17:33
@mikemhenry
Copy link
Contributor Author

@conda-forge/help-python , ready for review!

1 similar comment
@mikemhenry
Copy link
Contributor Author

@conda-forge/help-python , ready for review!

@ocefpaf
Copy link
Member

ocefpaf commented Oct 3, 2023

every multi output feedstock?

Probably not all of those but it doesn't hurt. It is a long standing bug in multiple outputs.

@mikemhenry
Copy link
Contributor Author

Awesome! Thanks @ocefpaf
We also don't care about the windows failure in this recipe.

@ocefpaf ocefpaf merged commit 0b23cbf into conda-forge:main Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants