Skip to content

Conversation

@banach-space
Copy link
Contributor

Adds missing arch name to the artifact name. Failing build that should
be fixed by this:

Adds missing arch name to the artifact name. Failing build that should
be fixed by this:
  * https://github.com/llvm/torch-mlir-release/actions/runs/19388930604/job/55479671779
@banach-space
Copy link
Contributor Author

CC @marbre - could you land this when you get a chance? Thanks!

Copy link
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

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

Artifacts for x86 currently would be have x64_64, please fix this in the matrix if we want to add the arch to the artifact name.

@banach-space
Copy link
Contributor Author

banach-space commented Nov 16, 2025

Artifacts for x86 currently would be have x64_64, please fix this in the matrix if we want to add the arch to the artifact name.

Why would this be a problem? Or, what would be the correct name?

That's a genuine question, I am not that familiar with different X86 extensions, I just assumed that X86_64 is the 64-bit version, which is what we use here, no? I am happy to change it, but at the some time want to avoid extra complexity that's not required.

For example, note that the current wheels already contain x86_64: https://github.com/llvm/torch-mlir-release/releases/expanded_assets/dev-wheels (e.g. torch_mlir-20251004.590-cp312-cp312-manylinux2014_x86_64.manylinux_2_17_x86_64.whl).

Also, I have a suspicion that the bit that I am updating, i.e. https://github.com/llvm/torch-mlir-release/blob/main/.github/workflows/buildReleaseAndPublish.yml#L57-L62, is not used/needed at all. Specifically, note this name from the action file:

name: snapshot-${{ matrix.package }}-${{ matrix.py_version }}-${{ env.tm_package_version }}

That's completely different to the actual wheels names from https://github.com/llvm/torch-mlir-release/releases/expanded_assets/dev-wheels, e.g.:

torch_mlir-20251004.590-cp312-cp312-manylinux2014_x86_64.manylinux_2_17_x86_64.whl

I am trying to verify this here.

Thanks!

EDIT 17/11/25

Confirmed, Upload python wheels is not required to upload releases to https://github.com/llvm/torch-mlir-release/releases/expanded_assets/dev-wheels. However, it is required to create artefacts (screenshot from here):
Screenshot 2025-11-17 at 08 11 42

Question - do we need both releases and artefacts? If "yes", then why not use consistent naming?

@marbre
Copy link
Member

marbre commented Nov 17, 2025

In the matrix the entry is x64_64 but this must be x86_64. If it only impacts the artifacts and not the releases this doesn’t matter to much, or rather does not create invalid wheels, but should be fixed if adding it to the artifact name.

@rolfmorel
Copy link

rolfmorel commented Nov 17, 2025

While we are fixing the artifact name, can we switch from $DATE.$RELNUM to $DATE-$TORCH_MLIR_COMMIT for the package naming please? E.g. torch_mlir-20251117-c180509-cp312-cp312-manylinux_2_27_aarch64.manylinux_2_28_aarch64.whl rather than dev-wheels/torch_mlir-20251117.634-cp312-cp312-manylinux_2_27_aarch64.manylinux_2_28_aarch64.whl.

Makes it a bit easier to pinpoint what commit of torch-mlir we should look at in case we encounter issues with particular package versions.

@banach-space
Copy link
Contributor Author

In the matrix the entry is x64_64 but this must be x86_64.

Sorry, you wrote x64_64 in your message and I saw x86_64 (I didn't notice x64 vs x86) 🤦🏻 My bad, thanks for pointing out!

@banach-space
Copy link
Contributor Author

@marbre I fixed the name here: commit

While we are fixing the artifact name, can we switch from $DATE.$RELNUM to $DATE-$TORCH_MLIR_COMMIT for the package naming please? E.g. torch_mlir-20251117-c180509-cp312-cp312-manylinux_2_27_aarch64.manylinux_2_28_aarch64.whl rather than dev-wheels/torch_mlir-20251117.634-cp312-cp312-manylinux_2_27_aarch64.manylinux_2_28_aarch64.whl.

Makes it a bit easier to pinpoint what commit of torch-mlir we should look at in case we encounter issues with particular package versions.

Let me do it in a separate PR. The dev cycle with these CI changes is quite long and I want to avoid delaying this because of my potential typos 😅

@banach-space banach-space requested a review from marbre November 18, 2025 14:02
Copy link
Member

@marbre marbre left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me now.

Which regards to switching to a different versioning, from a quick glance, the proposed one is not a valid version specifier. See https://packaging.python.org/en/latest/specifications/version-specifiers/#version-specifiers.

@marbre marbre merged commit 11c0638 into llvm:main Nov 19, 2025
@rolfmorel
Copy link

Which regards to switching to a different versioning, from a quick glance, the proposed one is not a valid version specifier. See https://packaging.python.org/en/latest/specifications/version-specifiers/#version-specifiers.

Ah, yeah. I would like to propose torch_mlir-20251117+c180509-cp312-cp312-manylinux_2_27_aarch64.manylinux_2_28_aarch64.whl instead of torch_mlir-20251117.634-cp312-cp312-manylinux_2_27_aarch64.manylinux_2_28_aarch64.whl. This is in line with the MLIR python bindings package (modulo any remaining syntactic subtleties): https://github.com/llvm/eudsl/releases/tag/mlir-python-bindings

@marbre
Copy link
Member

marbre commented Dec 8, 2025

Which regards to switching to a different versioning, from a quick glance, the proposed one is not a valid version specifier. See https://packaging.python.org/en/latest/specifications/version-specifiers/#version-specifiers.

Ah, yeah. I would like to propose torch_mlir-20251117+c180509-cp312-cp312-manylinux_2_27_aarch64.manylinux_2_28_aarch64.whl instead of torch_mlir-20251117.634-cp312-cp312-manylinux_2_27_aarch64.manylinux_2_28_aarch64.whl. This is in line with the MLIR python bindings package (modulo any remaining syntactic subtleties): https://github.com/llvm/eudsl/releases/tag/mlir-python-bindings

Let's maybe discuss this as part of a draft PR or in a new issue.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants