Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from
+18 −13

Conversation

booxter
Copy link
Contributor

@booxter booxter commented May 6, 2025

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 [email protected]

@booxter booxter mentioned this pull request May 6, 2025
Merged
@booxter
Copy link
Contributor Author

booxter commented May 6, 2025

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
Copy link
Contributor

mergify bot commented May 21, 2025

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
Copy link
Contributor Author

booxter commented May 22, 2025

@Mergifyio rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented May 22, 2025

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
Copy link
Contributor

mergify bot commented May 28, 2025

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
JamesKunstle
JamesKunstle previously requested changes May 28, 2025
View reviewed changes
Copy link
Contributor

@JamesKunstle JamesKunstle left a comment

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
Copy link
Contributor

mergify bot commented Jun 4, 2025

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
Copy link
Contributor

@cdoern cdoern left a comment

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

@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
Copy link
Contributor Author

booxter commented Jun 4, 2025

@Mergifyio rebase

@booxter
fix: Rename variable to control PG timeout to not refer to NCCL
Loading
Loading status checks…
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 <[email protected]>
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 4, 2025

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdoern cdoern

@JamesKunstle JamesKunstle

At least 2 approving reviews are required to merge this pull request.

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.

None yet
3 participants
@booxter @cdoern @JamesKunstle