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

Fix downstream CI #432

Merged
merged 42 commits into from Sep 10, 2021
Merged

Fix downstream CI #432

merged 42 commits into from Sep 10, 2021

Conversation

ogrisel
Copy link
Contributor

@ogrisel ogrisel commented Aug 1, 2021

To make it independent of #431.

@ogrisel ogrisel added the ci downstream Signal the CI to run the test suite of all registered cloudpickle downstream projects. label Aug 1, 2021
@codecov
Copy link

codecov bot commented Aug 1, 2021

Codecov Report

Merging #432 (6a78999) into master (343da11) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #432   +/-   ##
=======================================
  Coverage   92.36%   92.36%           
=======================================
  Files           4        4           
  Lines         720      720           
  Branches      150      150           
=======================================
  Hits          665      665           
  Misses         34       34           
  Partials       21       21           

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 343da11...6a78999. Read the comment docs.

ogrisel added a commit to scikit-learn-inria-fondation/follow-up that referenced this pull request Sep 7, 2021
## August 31th, 2021

### Gael

* TODO: Jeremy's renewal, Chiara's replacement, Mathis's consulting gig

### Olivier

- input feature names: main PR [#18010](scikit-learn/scikit-learn#18010) that links into sub PRs
  - remaining (need review): [#20853](scikit-learn/scikit-learn#20853) (found a bug in `OvOClassifier.n_features_in_`)
- reviewing `get_feature_names_out`: [#18444](scikit-learn/scikit-learn#18444)
- next: give feedback to Chiara on ARM wheel building [#20711](scikit-learn/scikit-learn#20711) (needed for the release)
- next: assist Adrin for the release process
- next: investigate regression in loky that blocks the cloudpickle release [#432](cloudpipe/cloudpickle#432)
- next: come back to intel to write a technical roadmap for a possible collaboration

### Julien

 - Was on holidays
 - Planned week @ Nexedi, Lille, from September 13th to 17th
 - Reviewed PRs
     - [`#20567`](scikit-learn/scikit-learn#20567) Common Private Loss module
     - [`#18310`](scikit-learn/scikit-learn#18310) ENH Add option to centered ICE plots (cICE)
     - Others PRs prior to holidays
 - [`#20254`](scikit-learn/scikit-learn#20254)
     - Adapted benchmarks on `pdist_aggregation` to test #20254 against sklearnex
     - Adapting PR for `fast_euclidean` and `fast_sqeuclidean` on user-facing APIs
     - Next: comparing against scipy's 
     - Next: Having feedback on [#20254](scikit-learn/scikit-learn#20254) would also help
- Next: I need to block time to study Cython code.

### Mathis
- `sklearn_benchmarks`
  - Adapting benchmark script to run on Margaret
  - Fix issue with profiling files too big to be deployed on Github Pages
  - Ensure deterministic benchmark results
  - Working on declarative pipeline specification
  - Next: run long HPO benchmarks on Margaret

### Arturo

- Finished MOOC!
- Finished filling [Loïc's notes](https://notes.inria.fr/rgSzYtubR6uSOQIfY9Fpvw#) to find questions with score under 60% (Issue [#432](INRIA/scikit-learn-mooc#432))
    - started addressing easy-to-fix questions, resulting in gitlab MRs [#21](https://gitlab.inria.fr/learninglab/mooc-scikit-learn/mooc-scikit-learn-coordination/-/merge_requests/21) and [#22](https://gitlab.inria.fr/learninglab/mooc-scikit-learn/mooc-scikit-learn-coordination/-/merge_requests/22)
    - currently working on expanding the notes up to 70%
- Continued cross-linking forum posts with issues in GitHub, resulting in [#444](INRIA/scikit-learn-mooc#444), [#445](INRIA/scikit-learn-mooc#445), [#446](INRIA/scikit-learn-mooc#446), [#447](INRIA/scikit-learn-mooc#447) and [#448](INRIA/scikit-learn-mooc#448)

### Jérémie
- back from holidays, catching up
- Mathis' benchmarks
- trying to find what's going on with ASV benchmarks
  (asv should display the versions of all build and runtime depndencies for each run)

### Guillaume

- back from holidays
- Next:
    - release with Adrin
    - check the PR and issue trackers

### TODO / Next

- Expand Loïc’s notes up to 70% (Arturo)
- Create presentation to discuss my experience doing the MOOC (Arturo)
- Help with the scikit-learn release (Olivier, Guillaume)
- HR: Jeremy's renewal, Chiara's replacement (Gael)
- Mathis's consulting gig (Olivier, Gael, Mathis)
@pierreglaser
Copy link
Member

OK, I got the loky and ray test suites to pass, but I think I'm getting a possible regression for Python 3.7 with distributed: in particular, test_pickle_empty fails (but note that it passes on Python 3.8). Maybe @jakirkham has an idea? Will keep investigating ASAP.

@jakirkham
Copy link
Member

Great! Thank you Pierre 😄 Have prodded some folks to review that PR 🙂

@pierreglaser
Copy link
Member

Mmh, I'm getting another error from the distributed test suite: test_dont_steal_unknown_functions just failed. Is this test known to be flaky? Is this likely (un)related to cloudpickle? @jakirkham may I quickly pick your brain again?

@pierreglaser
Copy link
Member

Re-launching the CI to see how flaky this failure is.

@jakirkham
Copy link
Member

It is somewhat flaky ( dask/distributed#3574 ), but it looks like a fails fairly infrequently (seeing very few references to it). If it's causing us issues, we could skip here or I could propose skipping upstream. WDYT?

@pierreglaser
Copy link
Member

Thanks again @jakirkham. I'll skip the test directly in cloudpickle, I'm unsure of the importance of this test for distributed. I've also skipped test_pickle_empty so that dask/distributed#5303 does not block us from releasing cloudpickle.

However, because test_pickle_empty is serialization related, we should not skip it forever, and as soon as we merge this PR, I'll open another one that will the skipping of test_pickle_empty and that we shall merge once dask/distributed#5303 gets merged.

@jakirkham
Copy link
Member

At some level it likely uses pickle for some things. That said, it is at best indirectly relevant for cloudpickle. Skipping it long term shouldn't cause any issues

Agree it would be good to get test_pickle_empty fixed and tested again here

@pierreglaser
Copy link
Member

I hope that cloudpickle is not the original root behind test_dont_steal_unknown_functions's flakiness :D

@jakirkham
Copy link
Member

Hehe I highly doubt that 😄

Distributed testing is hard. I think the test suite has gotten more robust especially over the past couple years, but there remain tricky issues like that test 😅

@ogrisel
Copy link
Contributor Author

ogrisel commented Sep 10, 2021

Thank you very much @pierreglaser and @jakirkham for fixing the CI! Merging!

I think we can release cloudpickle!

@ogrisel ogrisel merged commit 1938572 into cloudpipe:master Sep 10, 2021
@ogrisel ogrisel deleted the fix-downstream-ci branch September 10, 2021 09:01
@jakirkham
Copy link
Member

Since test_pickle_empty was fixed in upstream Dask, have readded testing of it in PR ( #442 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci downstream Signal the CI to run the test suite of all registered cloudpickle downstream projects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants