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

Clarify that command-line switches need to be manually set in addprocs #50843

Merged
merged 4 commits into from
Feb 11, 2024

Conversation

evetion
Copy link
Sponsor Member

@evetion evetion commented Aug 8, 2023

Fixes #50673

also fixed a small typo in an unrelated doc page.

@Seelengrab Seelengrab added the domain:docs This change adds or pertains to documentation label Aug 8, 2023
@nsajko
Copy link
Contributor

nsajko commented Aug 21, 2023

There's three (!) unrelated changes in this PR. Why don't you separate the other two fixes into separate PRs?

@nsajko
Copy link
Contributor

nsajko commented Aug 21, 2023

Regarding the main change, it's a duplicate of #50480

@evetion
Copy link
Sponsor Member Author

evetion commented Aug 21, 2023

There's three (!) unrelated changes in this PR. Why don't you separate the other two fixes into separate PRs?

I'd argue that there is a single main change, documenting how --heap-size-hint (and other flags) work with addproces. That requires adding --heap-size-hint to the docs itself.

The only unrelated change is a one character typo fix that I came across, and I didn't feel it would block this PR, so I included it. I'm happy to split it out to another PR if requested.

Regarding the main change, it's a duplicate of #50480

I missed this PR by @KnutAM, nice work! Note it's not the main change in this PR, I'd say that's the interaction with addprocs.

@fingolfin
Copy link
Contributor

Unfortunately this now has a conflict. @evetion would you be willing to resolve that. I think afterwards (and with my other comment taken into account) I think this is in a good shape for merging?

@evetion
Copy link
Sponsor Member Author

evetion commented Feb 10, 2024

Unfortunately this now has a conflict. @evetion would you be willing to resolve that. I think afterwards (and with my other comment taken into account) I think this is in a good shape for merging?

Fixed now, and moved part of the PR to the Distributed package.

@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 10, 2024
vtjnash pushed a commit to JuliaLang/Distributed.jl that referenced this pull request Feb 10, 2024
Copy link
Sponsor Member

@inkydragon inkydragon left a comment

Choose a reason for hiding this comment

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

It looks like the current changes have nothing to do with --heap-size-hint, maybe a title change is needed, then LGTM.

@evetion evetion changed the title Document --heap-size-hint and its interaction with addprocs Clarify that command-line switches need to be manually set in addprocs Feb 11, 2024
@evetion
Copy link
Sponsor Member Author

evetion commented Feb 11, 2024

It looks like the current changes have nothing to do with --heap-size-hint, maybe a title change is needed, then LGTM.

Thanks for the suggestion, I've updated the title.

@inkydragon inkydragon merged commit a37d626 into JuliaLang:master Feb 11, 2024
9 checks passed
@inkydragon inkydragon removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 11, 2024
samtkaplan pushed a commit to samtkaplan/Distributed.jl that referenced this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interaction between addprocs and --heap-size-hint
6 participants