[ROCm] Use CI env variable instead of Githubactions and CircleCI. #7501
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7501
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 FailuresAs of commit c6cb28c: NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @lcskrishna! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
There was a problem hiding this comment.
Hey @lcskrishna and thanks for the PR. I'm a little confused about the purpose of it though: you state
This PR fixes the ROCm CI to run same tests as CUDA CI.
but as of now, we aren't running any ROCm CI. What is your use case here as this patch won't do anything for us?
| @@ -28,7 +28,7 @@ | |||
| from torchvision.transforms.v2.functional import convert_dtype_image_tensor, to_image_tensor | |||
|
|
|||
|
|
|||
| IN_OSS_CI = any(os.getenv(var) == "true" for var in ["CIRCLECI", "GITHUB_ACTIONS"]) | |||
| IN_OSS_CI = bool(os.getenv('CI')) | |||
There was a problem hiding this comment.
Not using "CI" was a intentional choice: #6970 (comment)
There was a problem hiding this comment.
Hey @lcskrishna and thanks for the PR. I'm a little confused about the purpose of it though: you state
This PR fixes the ROCm CI to run same tests as CUDA CI.
but as of now, we aren't running any ROCm CI. What is your use case here as this patch won't do anything for us?
Hi @pmeier,
You are right that there is no ROCm CI on torchvision currently (we hope to change that in the near future!). However, the use-case is us trying to run internal CI on Jenkins for torchvision, and observing that some unit tests are not skipped for ROCm but skipped for upstream because of the above logic specific to CI platforms.
There was a problem hiding this comment.
Not using
"CI"was a intentional choice: #6970 (comment)
Honestly, that comment seems to be based on uncertainty/doubt rather than an actual issue with using the "CI" env variable :). We have some real examples of the PyTorch project using the "CI" env var without it interfering with Meta internal systems (afaik):
https://github.com/pytorch/pytorch/blob/master/tools/test/test_upload_test_stats.py#L6
https://github.com/pytorch/pytorch/blob/master/torch/testing/_internal/common_utils.py#L100
I'm not sure if Meta internal systems for torch and torchvision are different, but could we investigate it a bit more before concluding that "CI" won't work? It would make this code more "standard" and CI-vendor-agnostic. Thanks for considering!
There was a problem hiding this comment.
You are right that there is no ROCm CI on torchvision currently (we hope to change that in the near future!). However, the use-case is us trying to run internal CI on Jenkins for torchvision, and observing that some unit tests are not skipped for ROCm but skipped for upstream because of the above logic specific to CI platforms.
If this is just temporary and internal issue, can't you simply set one of the environment variables to fake being in such an environment?
I'm not sure if Meta internal systems for torch and torchvision are different, but could we investigate it a bit more before concluding that "CI" won't work? It would make this code more "standard" and CI-vendor-agnostic.
I'm in no position to perform such an investigation as I don't have access to Meta internal CI. However, I know that is was very deliberate to separate OSS, i.e. external, and internal CI. This is why we have this flag in the first place.
There was a problem hiding this comment.
@NicolasHug added this logic. But before we start any investigations, could you please answer my question above?
If this is just temporary and internal issue, can't you simply set one of the environment variables to fake being in such an environment?
There was a problem hiding this comment.
If this is just temporary and internal issue, can't you simply set one of the environment variables to fake being in such an environment?
@pmeier Yes, most certainly. This PR is just meant to use a more standard approach of identifying a CI environment in general. in line with what is done in upstream PyTorch, which also uses Meta internal CI.
There was a problem hiding this comment.
@lcskrishna @jithunnair-amd thanks for the PR.
The pytorch and torchvision CIs are setup somewhave differently both in OSS and internally, so something that works in pytorch core may not work the same way for torchvision.
As @pmeier mentioned, the explicit use of "CIRCLECI", "GITHUB_ACTIONS" instead of just "CI" is here to prevent any potential conflict with the internal CI. For example the internal CI may at some point decide to also set that variable on internal infra; that could lead to silent error or tests being skipped and it could take a while to diagnose, so we want to avoid such a scenario.
There was a problem hiding this comment.
@NicolasHug Thanks for the explanation. We'll close this PR.
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
This PR fixes the ROCm CI to run same tests as CUDA CI.
Summary:
Both the Circle CI environment and Github enviornment uses "CI" env variable set to true, as per the documentation below:
The same has been already used in PyTorch CI as well.
https://circleci.com/docs/variables/
https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables
cc: @jithunnair-amd
cc @jeffdaily @jithunnair-amd