Skip to content

Conversation

@charlesbluca
Copy link
Collaborator

As part of the new release, we made some modifications to the conda recipe using grayskull, so that it now generally matches up with the setup.py. This PR reflects some of those changes on the nightly recipe, and also makes some updates to the setup.py so that it is consistent with the desired conda recipe.

entry_points:
- dask-sql-server = dask_sql.server.app:main
- dask-sql = dask_sql.cmd:main
noarch: python
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we still build nightlies noarch even though the stable releases don't do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

No this should be dropped. It doesn't work with the selector below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just for clarity, does this mean that this package will only be available to the architecture + Python version used to build here (linux-64, py3.8)? If so then we might want to open a larger issue on who is using the nightlies to assess if this is desired or if we have to rethink our workflow.

If not, then we should be good to just remove this line and proceed

Copy link
Contributor

Choose a reason for hiding this comment

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

Will need to use variants to construct a build matrix for the Python versions

No easy answers for OS other than including more jobs on other OSes

Am assuming we are the primary users of nightlies. So would use that as a guiding principle until someone else asks for something more

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the link - I'll play around with variants for now, and check in with RAPIDS folk to see if we have any specific needs here.

setup(
name="dask_sql",
description="Dask SQL",
description="SQL query layer for Dask",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just noting that this change maps to the conda recipe's summary.

setup_requires=["setuptools_scm"] + sphinx_requirements,
install_requires=[
"dask[dataframe,distributed]==2021.10.0",
"dask[dataframe,distributed]>=2021.10.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll need this if we want to use unpinned versions of Dask with the nightlies for certain features (dask-sql-server in particular)

@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2021

Codecov Report

Merging #308 (ead3d94) into main (2194a75) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #308   +/-   ##
=======================================
  Coverage   95.75%   95.75%           
=======================================
  Files          65       65           
  Lines        2802     2802           
  Branches      418      418           
=======================================
  Hits         2683     2683           
  Misses         77       77           
  Partials       42       42           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2194a75...ead3d94. Read the comment docs.

Copy link
Contributor

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Given the recipe changes, noarch needs to be dropped

entry_points:
- dask-sql-server = dask_sql.server.app:main
- dask-sql = dask_sql.cmd:main
noarch: python
Copy link
Contributor

Choose a reason for hiding this comment

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

No this should be dropped. It doesn't work with the selector below

- pygments
- nest-asyncio
- tabulate
- importlib-metadata # [py<38]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the selector that makes noarch not an option

Copy link
Contributor

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

This should just work if the variant name shares the package name

Copy link
Collaborator Author

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Missed something

entry_points:
- dask-sql-server = dask_sql.server.app:main
- dask-sql = dask_sql.cmd:main
string: py{{ py_version.replace(".", "") }}_{{ GIT_DESCRIBE_HASH }}_{{ GIT_DESCRIBE_NUMBER }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot about this - is there a clean way to get a Python version for the package string from the build variant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can just replace with python instead of py_version. Though maybe you have already done this :)

@@ -1,6 +1,5 @@
{% set name = "dask-sql" %}
{% set version = environ.get('GIT_DESCRIBE_TAG', '0.0.0.dev') + environ.get('VERSION_SUFFIX', '') %}
{% set py_version=environ.get('CONDA_PY', 36) %}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If not will probably need to add this back in and modify

@charlesbluca
Copy link
Collaborator Author

Maybe we need to do the replace in a set template at the top of the file?

@jakirkham
Copy link
Contributor

Shouldn't be needed

@charlesbluca
Copy link
Collaborator Author

Think the issue might be that we need to use a replace filter here? If these are jinja2 templates that is

@jakirkham
Copy link
Contributor

Yeah saw the latest commit. That seems like what is needed

Copy link
Contributor

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Couple minor suggestions

@charlesbluca
Copy link
Collaborator Author

Looks like the conda build job takes roughly twice as long, which isn't as bad as I was expecting. I think this should be good to merge in pending tests, and we can follow up with a different option if other OSes are desired (could we do MacOS / Windows builds on gpuCI?)

If timing ever becomes an issue, I wonder if it's possible / safe to only build the package for one Python version on PRs

@jakirkham
Copy link
Contributor

Thanks Charles! 😄

@jakirkham jakirkham merged commit ae499a5 into dask-contrib:main Nov 11, 2021
@jakirkham
Copy link
Contributor

Yeah to your point above about build time. We could separate out the matrix onto multiple CI jobs. This is effectively what conda-forge does. It contains a bunch of variants and builds each one on a separate CI job. It's certainly something we could pursue if it becomes an issue.

Big picture, think it really all depends how long we maintain the Python 3.7 build here. Guessing we will want to add 3.9 & 3.10, which would increase the build matrix and may motivating dropping older Python versions. Dask (and others) may also choose to drop 3.7 when adding 3.10, which would push us in the same direction. Once 3.7 is dropped we don't really need the importlib-metadata dependency and can return to noarch (assuming we don't add any other Python version specific dependencies). At which point we don't really need to worry about more sophisticated builds.

@charlesbluca charlesbluca deleted the update-nightly-recipe branch January 19, 2022 21:23
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