Skip to content

fix: Rename variable to control PG timeout to not refer to NCCL#534

Closed
booxter wants to merge 1 commit into
instructlab:mainfrom
booxter:rename-timeout-var
Closed

fix: Rename variable to control PG timeout to not refer to NCCL#534
booxter wants to merge 1 commit into
instructlab:mainfrom
booxter:rename-timeout-var

Conversation

@booxter

@booxter booxter commented May 6, 2025

Copy link
Copy Markdown
Contributor

The variable is not backend specific. In the future, if/when we support
other backends, this will become more evidently a problem.

Signed-off-by: Ihar Hrachyshka ihar.hrachyshka@gmail.com

@booxter booxter mentioned this pull request May 6, 2025
Merged
@booxter

booxter commented May 6, 2025

Copy link
Copy Markdown
Contributor Author

Holding as draft until #521 merges.

@mergify mergify Bot added CI/CD Affects CI/CD configuration documentation Improvements or additions to documentation testing Relates to testing ci-failure labels May 6, 2025
@booxter booxter marked this pull request as ready for review May 21, 2025 14:49
@mergify

mergify Bot commented May 21, 2025

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @booxter please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label May 21, 2025
@booxter booxter force-pushed the rename-timeout-var branch from 11ad117 to 8ae1e8b Compare May 21, 2025 14:55
@mergify mergify Bot removed the needs-rebase label May 21, 2025
@booxter

booxter commented May 22, 2025

Copy link
Copy Markdown
Contributor Author

@Mergifyio rebase

@mergify

mergify Bot commented May 22, 2025

Copy link
Copy Markdown
Contributor

rebase

✅ Branch has been successfully rebased

@booxter booxter force-pushed the rename-timeout-var branch from 8ae1e8b to f5e4014 Compare May 22, 2025 17:58
@mergify mergify Bot added ci-failure and removed ci-failure labels May 22, 2025
@mergify

mergify Bot commented May 28, 2025

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @booxter please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label May 28, 2025
JamesKunstle
JamesKunstle reviewed May 28, 2025
View reviewed changes
Comment thread README.md Outdated
JamesKunstle
JamesKunstle previously requested changes May 28, 2025
View reviewed changes

@JamesKunstle JamesKunstle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

single change requested for clarity

@booxter booxter force-pushed the rename-timeout-var branch from f5e4014 to ad7964e Compare May 28, 2025 20:38
@mergify mergify Bot removed the ci-failure label May 28, 2025
@booxter booxter force-pushed the rename-timeout-var branch 3 times, most recently from 0a471be to 2fcb234 Compare May 28, 2025 21:49
@mergify mergify Bot removed the needs-rebase label May 28, 2025
@booxter booxter requested a review from JamesKunstle May 28, 2025 22:14
@booxter booxter force-pushed the rename-timeout-var branch from 2fcb234 to 14758ed Compare June 4, 2025 13:40
@mergify

mergify Bot commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @booxter please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Jun 4, 2025
cdoern
cdoern approved these changes Jun 4, 2025
View reviewed changes

@cdoern cdoern left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm, thanks for making it more accurate. Added one comment that may just be GH being weird with the diff

Comment thread README.md Outdated
@mergify mergify Bot added the one-approval label Jun 4, 2025
@booxter booxter force-pushed the rename-timeout-var branch from 14758ed to 13250a8 Compare June 4, 2025 17:48
@mergify mergify Bot removed the needs-rebase label Jun 4, 2025
@booxter booxter force-pushed the rename-timeout-var branch from 13250a8 to b1804a9 Compare June 4, 2025 17:49
@mergify mergify Bot added the ci-failure label Jun 4, 2025
@booxter booxter force-pushed the rename-timeout-var branch from b1804a9 to a43903c Compare June 4, 2025 17:50
@mergify mergify Bot added ci-failure and removed ci-failure labels Jun 4, 2025
@booxter

booxter commented Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

@Mergifyio rebase

@booxter
fix: Rename variable to control PG timeout to not refer to NCCL
4463754 
The variable is not backend specific. In the future, if/when we support
other backends, this will become more evidently a problem.

Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com>
@mergify

mergify Bot commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

rebase

✅ Branch has been successfully rebased

@booxter booxter force-pushed the rename-timeout-var branch from a43903c to 4463754 Compare June 4, 2025 22:47
@booxter booxter requested review from a team and removed request for JamesKunstle June 18, 2025 17:28
@booxter booxter dismissed JamesKunstle’s stale review June 18, 2025 17:29

Obsolete review. James is no longer working on the project.

@booxter booxter added hold and removed hold labels Jun 23, 2025
@booxter

booxter commented Jul 11, 2025

Copy link
Copy Markdown
Contributor Author

Feel free to post it as a new PR if still needed.

@booxter booxter closed this Jul 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cdoern cdoern cdoern approved these changes
+1 more reviewer
@JamesKunstle JamesKunstle JamesKunstle left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CI/CD Affects CI/CD configuration documentation Improvements or additions to documentation one-approval testing Relates to testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@booxter @cdoern @JamesKunstle