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

Prepare for ChunkSplitters 3.0 #119

Merged
merged 16 commits into from
Sep 26, 2024
Merged

Prepare for ChunkSplitters 3.0 #119

merged 16 commits into from
Sep 26, 2024

Conversation

carstenbauer
Copy link
Member

@carstenbauer carstenbauer commented Sep 20, 2024

OhMyThreads API Changes:

  • split can now be a Split or Symbol.
  • we now re-export Split, Consecutive, RoundRobin
  • we now re-export chunk, chunk_indices
  • when users provide chunks or index_chunks don't throw a warning about the fact that we auto turn off our internal chunking. instead throw a warning if they set incompatible chunking related kwargs.

TODOs:

  • update "non-exported" section of the docs
  • throw warning if user provides chunks or index_chunks and also sets @set chunking=true or other chunking options like @set ntasks=5 etc.

@carstenbauer
Copy link
Member Author

carstenbauer commented Sep 20, 2024

If one uses the linked PR of ChunkSplitters, all tests are passing locally.

@carstenbauer carstenbauer marked this pull request as draft September 20, 2024 15:57
@carstenbauer carstenbauer marked this pull request as ready for review September 25, 2024 19:19
@carstenbauer
Copy link
Member Author

@MasonProtter would be great if you could give this a quick look. Would like to merge and release this in the next couple of days.

Copy link
Member

@MasonProtter MasonProtter left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just some minor nitpicks.

CHANGELOG.md Show resolved Hide resolved
src/OhMyThreads.jl Show resolved Hide resolved
src/schedulers.jl Outdated Show resolved Hide resolved
src/schedulers.jl Outdated Show resolved Hide resolved
src/schedulers.jl Outdated Show resolved Hide resolved
src/schedulers.jl Outdated Show resolved Hide resolved
carstenbauer and others added 3 commits September 26, 2024 07:49
Co-authored-by: Mason Protter <mason.protter@icloud.com>
Co-authored-by: Mason Protter <mason.protter@icloud.com>
@carstenbauer
Copy link
Member Author

Apart from the const vs using discussion above, this seems to be good to go.

(What one could do is walk over the docs and see if any of the examples could be simplified by using chunks instead of index_chunks. But that's neither important nor does it have to be done in this PR.)

docs/src/refs/api.md Show resolved Hide resolved
src/schedulers.jl Outdated Show resolved Hide resolved
src/schedulers.jl Outdated Show resolved Hide resolved
src/schedulers.jl Outdated Show resolved Hide resolved
src/schedulers.jl Outdated Show resolved Hide resolved
carstenbauer and others added 4 commits September 26, 2024 20:06
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
@carstenbauer
Copy link
Member Author

I'll merge this. We don't have to register the new version right away to maybe make other breaking changes (e.g. related to #124).

@carstenbauer carstenbauer merged commit 4b91f66 into master Sep 26, 2024
16 checks passed
@carstenbauer carstenbauer deleted the cb/threepointo branch September 26, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants