-
Notifications
You must be signed in to change notification settings - Fork 4
[rocm-main]: Canonicalize python build versions #321
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
base: rocm-main
Are you sure you want to change the base?
Conversation
09d7a49
to
078c4d4
Compare
@@ -50,6 +59,7 @@ def create_manylinux_build_image(rocm_version, rocm_build_job, rocm_build_num): | |||
"--build-arg=ROCM_BUILD_JOB=%s" % rocm_build_job, | |||
"--build-arg=ROCM_BUILD_NUM=%s" % rocm_build_num, | |||
"--tag=%s" % image_name, | |||
"--build-arg=GPU_DEVICE_TARGETS=%s" % " ".join(gpu_device_targets), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably shouldn't be a part of this PR, but in the future, could we make GPU_DEVICE_TARGETS a comma-separated list everywhere? It confuses the Python arg parser if you use spaces, and that way we don't have to worry about which script needs what
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its intended to be a comma separated one, but since folks keep doing weird things I made the parser deal with both.
This part is actually setting an environment variable in the Docker build for the wheel image, so its not used by any python down the stack from here. (I believe it just gets printf'd to a file in the Dockerfile)
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import unittest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason that we're preferring unittest over pytest? pytest is the newer thing to use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytest extends unittest, so this works with pytest as well.
All unit test things in python extend from unittest
since like python 2.7 timeframe, so I just use that base stuff as its the most general, and the fancy features are rarely needed for my test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. Do we plan on running this in a separate Actions workflow in a future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I thought about that as I was adding to these, but its mostly just so I can make sure my parser functions actually do what they are supposed to when developing them. i.e. running locally
I think it would be easier to do when we have things move over to the plugin repo.
r = ci_build.parse_gpu_targets(" ".join(targets)) | ||
self.assertEqual(r, targets) | ||
|
||
r = ci_build.parse_gpu_targets(",".join(targets)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be convinced otherwise, but it makes the test case easier to interpret if we follow a "one test case, one assertion" rule (unless you need to do multiple checks on the same output, check for None, etc). Could we make the " " and "," cases separate test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I cheated here, but this is the "good path" cases vs the negative path cases which are below, and have specific failure conditions.
I split the difference between one test case for everything vs one test case per assert and ended up with this compromise lol
078c4d4
to
d70840d
Compare
Move defaults for these up to ci_build python We also add some error catching logic in the python script to clean up weirdness coming from the build scripts. Also clean up some formatting and typos (cherry picked from commit ef1d561)
The ci_build parsing will now drop the revision portion of the requested python version targets to build for, since users were passing in old buggy versions. Using only major.minor will result in the latest of the minor series being used, and won't affect any build outputs like bytecode or ABI bindings. Those are tied to major.minor versions of CPython.
d70840d
to
5bf0875
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for separating out the test cases
The ci_build parsing will now drop the revision portion of the requested python
version targets to build for, since users were passing in old buggy versions.
Using only major.minor will result in the latest of the minor series being used, and won't affect
any build outputs like bytecode or ABI bindings.
Also this PR includes a port of the gpu device target fixes and tests, so I could add more tests.