Skip to content

Conversation

amdfaa
Copy link
Contributor

@amdfaa amdfaa commented Jan 26, 2023

@amdfaa amdfaa requested a review from a team as a code owner January 26, 2023 00:50
@pytorch-bot
Copy link

pytorch-bot bot commented Jan 26, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/93032

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 7288d74:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added module: rocm AMD GPU support for Pytorch topic: not user facing topic category labels Jan 26, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 26, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@huydhn
Copy link
Contributor

huydhn commented Jan 26, 2023

/easycla

Copy link
Contributor

@huydhn huydhn left a comment

Choose a reason for hiding this comment

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

Overall LGTM! But as this step is generic and can be used by others as well. It makes sense to move it to an action, i.e. https://github.com/pytorch/pytorch/blob/master/.github/actions/get-workflow-job-id/action.yml, then invoke the action wherever it is needed. The action could accept a diskspace cutoff param defaults to 70%

Also there is a minor linter check.

@huydhn
Copy link
Contributor

huydhn commented Jan 26, 2023

Also please sign the CLA first

@amdfaa
Copy link
Contributor Author

amdfaa commented Jan 26, 2023

I'm signing the easycla but it's not updating on the ticket. I submitted a help request for the issue.

@huydhn
Copy link
Contributor

huydhn commented Jan 26, 2023

I'm signing the easycla but it's not updating on the ticket. I submitted a help request for the issue.

Oh once you have signed it, I would just need to invoke the check again to make it pass

@amdfaa
Copy link
Contributor Author

amdfaa commented Jan 26, 2023

Sounds good! Please check if it works.

@huydhn
Copy link
Contributor

huydhn commented Jan 26, 2023

/easycla

@huydhn
Copy link
Contributor

huydhn commented Jan 26, 2023

Weird, invoking /easycla doesn't seem to do anything. You're right to submit a help request. May be also confirm that: your email is the correct one used in your GitHub account.

Copy link
Contributor

@malfet malfet 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, but please make sure to sign CLA (PR authors email must be covered by CLA)

@amdfaa
Copy link
Contributor Author

amdfaa commented Jan 26, 2023

Weird, invoking /easycla doesn't seem to do anything. You're right to submit a help request. May be also confirm that: your email is the correct one used in your GitHub account.

Sounds good!

Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Hmm, actually, any tips how one can test it?
Because, afaik $diskspace -ge diskspace_cutoff would always evaluate to true, aren't it?

$ diskspace=1 ;if [[ $diskspace -ge diskspace_cutoff ]]; then echo "Ha ha ha $diskspace"; fi
Ha ha ha 1

@malfet
Copy link
Contributor

malfet commented Jan 26, 2023

@ZainRizvi , Hmm, don't we have lintrunner to detect such mistakes?

@ZainRizvi
Copy link
Contributor

ZainRizvi commented Jan 26, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@ZainRizvi
Copy link
Contributor

@ZainRizvi , Hmm, don't we have lintrunner to detect such mistakes?

@kit1980, do we have a shell lintrunner to catch issues like the one mentioned here?

@amdfaa
Copy link
Contributor Author

amdfaa commented Jan 26, 2023

The support team stated the following: To resolve this [issue], the community usually make sure the GitHub Global Configs are properly configured, and run a git rebase to update the committer/author information on that specific commit.

@huydhn
Copy link
Contributor

huydhn commented Jan 27, 2023

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased rocm-diskspace-check onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout rocm-diskspace-check && git pull --rebase)

@huydhn
Copy link
Contributor

huydhn commented Jan 27, 2023

/easycla

@jithunnair-amd
Copy link
Collaborator

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased rocm-diskspace-check onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout rocm-diskspace-check && git pull --rebase)

Copy link
Contributor

@huydhn huydhn left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for fixing this issue!

@jithunnair-amd jithunnair-amd added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 2, 2023
@jithunnair-amd
Copy link
Collaborator

@amdfaa Added ciflow/trunk label to trigger ROCm test jobs on trunk workflow. Please check them when they finish to see that the diskspace check is being executed correctly.

@amdfaa
Copy link
Contributor Author

amdfaa commented Feb 3, 2023

Hmm, actually, any tips how one can test it?

I have locally tested this pr on jwr-20 with a lower cutoff and the script executed successfully.

@jithunnair-amd
Copy link
Collaborator

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

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

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged module: rocm AMD GPU support for Pytorch open source test-config/default topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ROCm machines out of disk space

9 participants