-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Release 2021.11.0 with grayskull #28
Release 2021.11.0 with grayskull #28
Conversation
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipe:
|
number: 0 | ||
script: {{ PYTHON }} -m pip install . --no-deps -vv | ||
skip: true # [py2k] |
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.
This seems to be added because of the selector below. So makes sense
Would suggest taking the dependencies before and after and sorting them. Then doing a diff on those. That should help clarify what has actually changed. Also raised issue ( conda/grayskull#259 ) on keeping these in the original order |
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 ( |
Resorted the dependencies here to match the original ordering to simplify the diff |
- python >=3.6 | ||
- setuptools_scm | ||
- python | ||
- setuptools-scm |
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.
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.
Thanks Charles! 😄
Raised a few issues based on trying things here. Made a few observations above. Some are actionable. Though some are not.
- pandas >=1.0.0 | ||
- jpype1 >=1.0.2 | ||
- openjdk >=8 |
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.
Obviously we want to keep this. Raised issue ( conda/grayskull#261 )
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.
Do we also want to keep the maven
build requirement?
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.
Sorry missed that. Agree we want to keep that. Commented above
- prompt_toolkit >=3.0.8 | ||
- pygments >=2.7.3 | ||
- nest-asyncio >=1.0.0 | ||
- tabulate >=0.8.9 | ||
- prompt-toolkit | ||
- pygments | ||
- nest-asyncio | ||
- tabulate |
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.
While it may seem surprising that these constraints were dropped, that did happen in the setup.py
. So this is expected behavior (at least from Grayskull). Whether they should or shouldn't be there upstream is another question
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.
Good point - I notice that those packages are constrained by the CI environment so I can open an issue to keep track of making those consistent for the next release
- pygments | ||
- nest-asyncio | ||
- tabulate | ||
- importlib-metadata # [py<38] |
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.
This is picked up from upstream and expected. This is also why the package can no longer be noarch
recipe/meta.yaml
Outdated
|
||
test: | ||
imports: | ||
- dask_sql | ||
- dask_sql.input_utils |
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.
Not sure why this is here. We can drop this. Raised issue ( conda/grayskull#262 )
- dask-sql-server --help | ||
- dask-sql --help |
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.
These seem like a good idea to check :)
requires: | ||
- pip | ||
|
||
about: | ||
home: http://github.com/nils-braun/dask-sql/ | ||
summary: SQL Query Layer for dask | ||
home: http://github.com/dask-contrib/dask-sql/ |
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.
Glad it caught this 😄
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.
It actually didn't catch this one 🙁 this was one of the few things I changed manually
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.
Interesting if I run this locally, Grayskull seems to get this right. It should just be grabbing it from here. Could you please recheck?
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.
Ah yeah my bad, I'm thinking of the change I made to the summary - this was caught by grayskull 🎉
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.
Ah ok. In that case we might want to update this line
We should also re-render now that the package is not |
Adding to arch migrations ( conda-forge/conda-forge-pinning-feedstock#2160 ) since this will be not be noarch any more. This should help us provide packages on other architectures (like ARM) |
recipe/meta.yaml
Outdated
build: | ||
- maven >=3.6.0 |
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.
Yes please keep this too
Windows Python 3.9 failures appear to be due to missing |
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.
Thanks for the comments @jakirkham and the work here along with grayskull @charlesbluca :)
@conda-forge-admin, please restart CI |
Hi! This is the friendly conda-forge automerge bot! I considered the following status checks when analyzing this PR:
Thus the PR was passing and merged! Have a great day! |
Rerunning CI now that |
Noticed a clobber warning in the log. Mentioned here ( conda-forge/openjdk-feedstock#59 (comment) ) |
Looks like packages are up and available from the CDN 🙂 |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)Closes #29
Main change we need here is the Dask pinning in the first commit, the rest is optional suggestions made when generating the recipe with grayskull.
cc @quasiben