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

Improve documentation for deprecated methods that only make sense for parallel collections #12900

Closed
SethTisue opened this issue Nov 6, 2023 · 11 comments · Fixed by scala/scala#10605

Comments

@SethTisue
Copy link
Member

SethTisue commented Nov 6, 2023

at scala/toolkit#31 (comment) @philipschwarz noted:

Maybe instead of just saying that the aggregate function
is not relevant for sequential collections. Use `foldLeft(z)(seqop) instead., "2.13.0
the [Scala Doc for aggregate]> (https://github.com/scala/scala/blob/v2.13.10/src/library/scala/collection/IterableOnce.scala#L1122) could also say that aggregate is relevant for parallel collections and it could provide a link to https://github.com/scala/scala-parallel-collections.

perhaps the method should also be deprecated? and perhaps there are other such methods? @scala/collections

@philipschwarz
Copy link

"perhaps the method should also be deprecated?" - but it is already deprecated, right?

@SethTisue SethTisue changed the title Deprecate methods that only make sense for parallel collections? Improve documentation for deprecated methods that only make sense for parallel collections Nov 6, 2023
@SethTisue SethTisue added the docs label Nov 6, 2023
@SethTisue
Copy link
Member Author

SethTisue commented Nov 6, 2023

@philipschwarz You are right. In that case, perhaps all that's needed is to adjust the deprecation message and/or Scaladoc for the deprecated method(s). I've adjusted the ticket title accordingly.

I never took the parallel programming course — does aggregate come up there, is that the concern?

@philipschwarz
Copy link

philipschwarz commented Nov 6, 2023

I never took the parallel programming course — does aggregate come up there, is that the concern?

yes, it starts coming up

  • on slide 38 of Data-Parallel Operations (this deck could be from an older version of the course, but is freely accessible)
  • on slide 18 of Data Parallel Operations II (this deck is from the current version of the course, but access to it may be time-limited or require access rights) - here is the page where the deck and a video can be found

@som-snytt
Copy link

I was just visiting the ticket that scala 2.13.12> :require scala-parallel-collections_2.13-1.0.4.jar doesn't work.

It would be amazing to have targeted API, where certain symbols are "unlocked" by certain values of --release. Or perhaps unlocked if certain packages are available. aggregate would be invisible in Scala unless unlocked.

Similarly, at least the parallel classes that "infect" core packages could be preloaded in REPL but invisible. The REPL use case was ad hoc unlocking, which would require symbols exist but filtered from lookup.

@ansvonwa
Copy link
Member

ansvonwa commented Nov 19, 2023

Actually, I do not really get why aggregate was deprecated in the first place: Wasn't the whole point of aggregate that I as a library/function author could use it to consume a collection independently of its internal structure?

Like "If someone hands me a parallel collection, it will be fast. If it's a non-parallel one (and maybe par collections aren't even on the classpath) it will still work as expected with expected performance. And if it's some very weird view of a distributed collection it will still be as fast as the collection allows."
Didn't this use case became even more relevant when par collections were removed from the std lib? Before, you could (if you really cared) pattern match on the collection type. But when par collections may not even be on the classpath, this is not really an option.

Or was it just that nobody really used it so it was deemed irrelevant?

@som-snytt
Copy link

Not an expert, but it doesn't make sense on IterableOnce because you'd have to traverse it just to partition it.

My other mantra is that deprecation doesn't entail removal, it just means "there is a better way". In this case, use aggregate on a more suitable type, namely, a parallel thingy. (ParIterableLike)

My PR to supply some missing doc did not detect aggregate, maybe because of the deprecation noise.

I think combop, which I would have named comboop but I'm not an expert, should be marked unused here, as well. I was wondering if Scaladoc should document unusedness. "Brittle inheritance" means you never know what an override will do.

The semantics of comboop is restrictive, so for pedagogy some doc is needed. I'm willing to take a swing at capturing pithily both ansvonwa's niche use case and the deprecated sense that if you don't know what you're doing, this is probably not the operation you want. I've forgotten what "Odersky level" corresponds to ansvonwa's use case. @Odersky(L4) we need an official annotation. Then scalac --odersky=L4.

Deprecation affects autocomplete? That may justify deprecation. Experts will hit tab twice, or whatever the secret gesture.

@som-snytt
Copy link

I did not search for more work to do here: scala/scala@1d0597c

@philipschwarz
Copy link

speaking of the aggregate function, I just finished making this yesterday and can't resist the temptation to share it here in case someone looking at this ticket might be interested in it: http://fpilluminated.com/assets/scala-left-fold-parallelisation-three-approaches.html
image

@philipschwarz
Copy link

@som-snytt FWIW, I left some comments in scala/scala@1d0597c, but only realised later that it was a commit rather than a PR.

@som-snytt
Copy link

som-snytt commented Nov 25, 2023

@philipschwarz thanks, I definitely read

IMHO this is a great improvement.

which I obviously remember clearly. I still have a mark where I patted myself on the back.

I think github didn't notify me about the other comments, but

the above are just some ideas - feel free to ignore

was not the reason I did not update. I agree with your clarifications, and thanks.

Edit: github doesn't navigate from that commit to PRs to which it belongs, which is scala/scala#10605

@SethTisue
Copy link
Member Author

in this space: scala/scala#10624

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants