Skip to content

[WIP] joblib 0.12 integration #9486

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[WIP] joblib 0.12 integration #9486

wants to merge 3 commits into from

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Aug 3, 2017 •
edited
Loading

This PR is not mean to be merged as this. This vendors the development branches of loky and joblib, notably the following PRs:

The goal is to have the full scikit-learn CI test suite run on this PRs to ensure that we can merge them and release them without causing a regression in scikit-learn master.

TODO for this PR:

  • fix the coverage bug upstream and backport a fix via a monkeypatch or other in our CI
  • decide whether we want to keep on vendoring everything or not

This is part of #7650: A plan to safely support OpenMP in our Cython code base.

@amueller
Copy link
Member

amueller commented Aug 3, 2017

Or we could stop vendoring and declare our dependencies?

@amueller
Copy link
Member

amueller commented Aug 3, 2017

(though that would still require a PR with the dependency changed to test the integration)

@amueller
Copy link
Member

amueller commented Aug 3, 2017 •
edited
Loading

wait joblib vendors loky?!? I'm against integrating loky either as vendored in joblib or as vendored in sklearn. Scikit-learn is not a package manager.

@ogrisel
Copy link
Member Author

ogrisel commented Aug 3, 2017

I agree that nowadays dependency management works much better that it used to do in the past so that we can decide to stop vendoring stuff. Personally I am fine with both approaches. All those packages (joblib, loky and cloudpickle) have universal wheels, there is no compiled extensions in them.

@ogrisel
Copy link
Member Author

ogrisel commented Aug 3, 2017

This PR has revealed that the new combinations of the long running threads and coverage can trigger the following bug in the coverage package:

https://bitbucket.org/ned/coveragepy/issues/581/44b1-44-breaking-in-ci

We did not trigger this has part of the joblib and loky CI setup for some reason.

@ogrisel
Copy link
Member Author

ogrisel commented Aug 3, 2017 •
edited
Loading

The appveyor tests of sklearn have revealed a packaging issue. I am on it but I have to go now. I will fix it tomorrow.

@ogrisel ogrisel force-pushed the loky-integration branch 2 times, most recently from 85c35d5 to 3e0916c Compare August 4, 2017 16:24
@ogrisel
Copy link
Member Author

ogrisel commented Aug 4, 2017

@tomMoral the appveyor failure (Python 2.7 / 64 bit) seems to reveal a real bug in loky:

Exception in thread QueueManager:
Traceback (most recent call last):
  File "C:\Python27-x64\lib\threading.py", line 801, in __bootstrap_inner
    self.run()
  File "C:\Python27-x64\lib\threading.py", line 754, in run
    self.__target(*self.__args, **self.__kwargs)
  File "C:\Python27-x64\lib\site-packages\sklearn\externals\joblib\externals\loky\process_executor.py", line 500, in _queue_management_worker
    timeout=_poll_timeout)
  File "C:\Python27-x64\lib\site-packages\sklearn\externals\joblib\externals\loky\backend\_win_wait.py", line 52, in wait
    elif h.poll(0):
AttributeError: 'long' object has no attribute 'poll'

I wonder why we did not get that bug from the loky appveyor CI.

@ogrisel
Copy link
Member Author

ogrisel commented Aug 4, 2017 •
edited
Loading

@tomMoral as shown in the second commit of tomMoral/loky#87 we did not test 64 bit windows as we thought we were. Will read up about tox and appveyor.

This was referenced Aug 4, 2017
Merged
Merged
@ogrisel
Copy link
Member Author

ogrisel commented Aug 8, 2017

The failures with 64 bit Python under windows have been fixed. CI is red because of the aforementioned coverage bug.

@ogrisel
Copy link
Member Author

ogrisel commented Aug 11, 2017

\o/ the coverage fix works. I hope my PR will get merged upstream in the coverage project ;)

@tomMoral tomMoral force-pushed the loky-integration branch 2 times, most recently from 923d766 to e2ca63b Compare June 22, 2018 10:22
@TomAugspurger TomAugspurger mentioned this pull request Jun 22, 2018
Closed
@ogrisel ogrisel changed the title [WIP] vendor loky + joblib dev branches to test sklearn integration [WIP] joblib 0.12 integration Jun 22, 2018
@TomAugspurger TomAugspurger mentioned this pull request Jul 19, 2018
Open
@ogrisel
Copy link
Member Author

ogrisel commented Jul 20, 2018

@tomMoral I am currently rebasing this PR on top of the current master (and upgrading to joblib 0.12.1).

@ogrisel ogrisel force-pushed the loky-integration branch 5 times, most recently from 778c2ff to bfb4aca Compare July 23, 2018 08:39
@scikit-learn scikit-learn deleted a comment from sklearn-lgtm Jul 23, 2018
@scikit-learn scikit-learn deleted a comment from sklearn-lgtm Jul 23, 2018
@scikit-learn scikit-learn deleted a comment from sklearn-lgtm Jul 23, 2018
@scikit-learn scikit-learn deleted a comment from sklearn-lgtm Jul 23, 2018
@ogrisel ogrisel force-pushed the loky-integration branch from ff50d08 to dd160f0 Compare July 24, 2018 08:44
@ogrisel
Copy link
Member Author

ogrisel commented Aug 3, 2018

Closing in favor of #11741.

@ogrisel ogrisel closed this Aug 3, 2018
@ogrisel ogrisel deleted the loky-integration branch August 3, 2018 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ogrisel @amueller