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

Start building pre-releases with cythonized scheduler #5831

Merged
merged 23 commits into from
Feb 24, 2022

Conversation

charlesbluca
Copy link
Member

This PR modifies the Distributed pre-release recipe to build Cython variants of Distributed using the steps outlined by @jakirkham in #4442 (comment).

Choosing which Distributed variant to install should be doable by specifying the distributed-proc build string during an install / env creation command, though with track_features="cythonized-scheduler" specified for Cython builds Conda should always prioritize non-Cython builds by default.

  • Closes #xxxx
  • Tests added / passed
  • Passes pre-commit run --all-files

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @charlesbluca. There was some talk during the last monthly community meeting of dropping the cythonized version of the scheduler (also xref #5685). My guess is we should wait for a decision on that before adding extra builds here

@jakirkham
Copy link
Member

James as part of that discussion we had decided that if some work was done here to setup nightlies and refactor out the Cythonized parts, we could keep it. This is one of those steps. However I think it is important for us to have the opportunity to progress here otherwise none of the actionables requested can complete. IOW it becomes a Catch 22

@jrbourbeau
Copy link
Member

jrbourbeau commented Feb 17, 2022

Ah, my mistake -- I thought we were just looking for progress on the refactor part. If nightlies were also requested, then let's carry on with the changes here 👍

@jakirkham
Copy link
Member

Right this was one of the issues Florian had raised in the issue above and was discussed in the meeting. So this is our attempt to address that.

Agree there is still refactoring work to do as well.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2022

Unit Test Results

       12 files  +       4         12 suites  +4   7h 15m 20s ⏱️ + 2h 39m 10s
  2 621 tests +     20    2 540 ✔️ +     31    81 💤  -     9  0  - 2 
15 650 runs  +5 270  14 762 ✔️ +5 055  888 💤 +217  0  - 2 

Results for commit 369088b. ± Comparison against base commit 60d82c2.

♻️ This comment has been updated with latest results.

Copy link
Member

@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.

Thanks Charles! 😄

Had a few suggestions below. Generally this seems to be going in a good direction.

continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
Copy link
Member 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.

Thanks for the thorough review @jakirkham 😄 a few responses:

continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
Copy link
Member

@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.

Added some suggestions above to make this noarch. We should be able to drop the conda convert lines with this

continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
Copy link
Member

@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.

Sorry for the 2nd review batch. Thinking this will resolve the CI issue

Copy link
Member

@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 is looking pretty good. Thanks for all of the work here Charles 😄

Had a couple suggestions related to how selectors and Jinja work. IIUC this should also fix the CI issue

continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
@@ -2,80 +2,102 @@
{% set new_patch = major_minor_patch[2] | int + 1 %}
{% set version = (major_minor_patch[:2] + [new_patch]) | join('.') + environ.get('VERSION_SUFFIX', '') %}
{% set dask_version = environ.get('DASK_CORE_VERSION', '0.0.0.dev') %}
{% set cython_enabled = build_ext == "cython" %}
Copy link
Member

Choose a reason for hiding this comment

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

Jinja unfortunately doesn't affect selectors. So we would need to perform this comparison in selectors

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 thanks for the clarification here - good to know for the future 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Ofc. Showed another way we could do this in this review ( #5831 (review) ). Though ok with either approach 🙂

continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
Copy link
Member

@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.

Not that we need to do this, but this is another way we could handle the Jinja/selector Cython portion

continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
continuous_integration/recipes/distributed/meta.yaml Outdated Show resolved Hide resolved
Copy link
Member 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.

Thanks for all the help here @jakirkham 😄 some questions w.r.t. shifting towards noarch, as I notice we are still building py38 / py39 packages:

Co-authored-by: jakirkham <jakirkham@gmail.com>
Copy link
Member

@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.

The different versions are still needed for Cython (though not pure Python). That said, maybe there is a way to simplify things if we use a skip. Would also want to check what Python versions are built against by default with Conda-Build

charlesbluca and others added 2 commits February 22, 2022 13:11
@jakirkham
Copy link
Member

Looks like it is doing the right thing now

INFO :: The inputs making up the hashes for the built packages are as follows:
{
  "distributed-2022.02.1a220222-py38_g293ef607_47_cython": {
    "recipe": {
      "c_compiler": "gcc",
      "cython_enabled": "True",
      "target_platform": "linux-64"
    }
  },
  "distributed-2022.02.1a220222-py38_g293ef607_47_python": {
    "recipe": {
      "cython_enabled": "False"
    }
  },
  "distributed-2022.02.1a220222-py39_g293ef607_47_cython": {
    "recipe": {
      "c_compiler": "gcc",
      "cython_enabled": "True",
      "target_platform": "linux-64"
    }
  },
  "distributed-2022.02.1a220222-py39_g293ef607_47_python": {
    "recipe": {
      "cython_enabled": "False"
    }
  },
  "distributed-impl-2022.02.1a220222-cython": {
    "recipe": {}
  },
  "distributed-impl-2022.02.1a220222-python": {
    "recipe": {}
  }
}

Copy link
Member

@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.

LGTM. Thanks Charles! 😄

@jakirkham
Copy link
Member

@jrbourbeau @jsignell, do either of you have more thoughts here? 🙂

@jsignell
Copy link
Member

No thoughts from me. I'm happy if you are happy :)

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Yep, same here, I'm happy if you're happy with the changes here.

(This is a non-blocking comment) I noticed the nightly CI build here takes ~10 minutes and is run on all pull requests and pushes to main. That seems like a significant amount of CI time to me. I'm wondering how much value there is in building that often vs. only on pushes to main or even a single nightly cron job. dask/dask and dask/distributed have been quite active recently and I've noticed longer wait times for CI builds to start. If we switched to only building on pushes to main or a nightly cron job, would folks using these builds miss out on much?

@jakirkham
Copy link
Member

There is probably still some value in building these on PRs that change the Conda packages in particular (though maybe this can be skipped for other PRs)

Otherwise building on main should be fine

@jrbourbeau
Copy link
Member

PRs that change the Conda packages in particular (though maybe this can be skipped for other PRs)

Ah, that sounds like a nice balance. It looks like GitHub has support for only triggering builds when certain files are modified https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#onpushpull_requestpull_request_targetpathspaths-ignore. Again, I don't mean for this to block this PR. We can make further updates in future PRs

@charlesbluca
Copy link
Member Author

Yeah was about to suggest the same method - I'm okay with pushing a quick commit doing this targetting the following files:

  • the conda recipes
  • setup.py
  • the workflow file controlling the pre-release builds

@jakirkham
Copy link
Member

@jrbourbeau how do the changes above look? 🙂

(thanks Charles for adding those! 🙏)

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Nice, LGTM -- thanks @charlesbluca

@jakirkham jakirkham merged commit 94ebd57 into dask:main Feb 24, 2022
@jakirkham
Copy link
Member

Thanks Charles for the PR and Julia & James for the reviews! 😄

@jakirkham
Copy link
Member

jakirkham commented Feb 24, 2022

Tested the file filtering logic in PR ( #5826 ) and it works as expected (Conda nightly build skipped)! 🎉

Edit: Also file filters work when modifying the recipes as in PR ( #5857 ) 🚀

@jakirkham jakirkham mentioned this pull request Apr 12, 2022
3 tasks
martindurant added a commit to martindurant/distributed that referenced this pull request Apr 12, 2022
@charlesbluca charlesbluca deleted the cython-builds branch July 20, 2022 03:00
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

4 participants