return_generator={True,False} -> return_as={'list','generator'}#1458
return_generator={True,False} -> return_as={'list','generator'}#1458tomMoral merged 10 commits intojoblib:masterfrom
return_generator={True,False} -> return_as={'list','generator'}#1458Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1458 +/- ##
==========================================
- Coverage 94.88% 94.87% -0.02%
==========================================
Files 45 45
Lines 7471 7474 +3
==========================================
+ Hits 7089 7091 +2
- Misses 382 383 +1
☔ View full report in Codecov by Sentry. |
tomMoral
left a comment
There was a problem hiding this comment.
LGTM, a few nitpicks. Happy to have the opinion of @GaelVaroquaux on this one.
|
Came here via the sprint/Franck at the sprint. How about |
|
As @adrinjalali mentioned it could be worth at least considering adding an extra method for different output type. Something like (but the name can certainly be better), joblib.Parallel(...).call_as_generator(delayed(sqrt)(i**2) for i in range(10)))It also feels that something that returns a variable type depending on the input parameter is not great API wise, and would confuse type checkers. I mean between list and iterators of results it could still work. But if you want to add an iterator of future results later that's a completely different return type. |
with At least it's very explicit. It's a bit verbose but I think I prefer this side of the tradeoff. |
If we ever go into returning "future" or "promise" objects I think we should introduce a completely new API (and probably mimic that of |
|
Some more online discussion later, we converge on (About futures/promises, returning such objects is definitely not in the scope of what Parallel offers, the misunderstanding comes from bad wording from me early on, sorry about that.) |
|
After discussion at the scikit-learn sprint, I also prefer the following:
This is explicit enough, technically correct and concise enough. |
return_generator={True,False} -> return_as={'list','submitted'}return_generator={True,False} -> return_as={'list','generator'}
|
Merging as the failure on |
Change the boolean
return_generatorkeyword, toreturn_asthat is expected to take values in{'list','submitted'}, to anticipate a future'completed'keyword when implementing #1449 .