Skip to content
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

WAR bad array_record wheel on arm64 #5109

Merged
merged 1 commit into from Nov 1, 2023

Conversation

nouiz
Copy link
Contributor

@nouiz nouiz commented Oct 14, 2023

This WAR a bug in array_record:
google/array_record#71

Mostly, all old pip wheel only work on x86_64, but get installed on all platform as the wheel is marked as platform independent.

A fix was merged in array_record, but no new wheel is made yet.
This PR make sure that we will crash the installation until a new release is done on all non-x86_64 platform. When the release is done, it will make sure to not install older version.

@fineguy fineguy self-assigned this Oct 30, 2023
@fineguy
Copy link
Collaborator

fineguy commented Oct 30, 2023

Hi @nouiz, thanks for your contribution!

I see that there's array-record 0.5.0: https://pypi.org/project/array-record/#history
Could you please confirm whether it has fixed your issue?

@nouiz
Copy link
Contributor Author

nouiz commented Oct 30, 2023

The release of array_record has a fix, but this doesn't fix the old packages that still install on ARM while they don't work on ARM.
So this PR is still needed.

What would remove the need forthis PR is if array_record publish an ARM container, or remove all old wheel. But they didn't.
Merging this PR will be much quicker than getting ARM container for array_record.

@nouiz
Copy link
Contributor Author

nouiz commented Oct 30, 2023

We could also bump the min version for all platform to 0.5 if you prefer. This will fix the issue I'm targetting while being a smaller diff.

@fineguy
Copy link
Collaborator

fineguy commented Oct 31, 2023

Yes, I think bumping the min version is better. Could you please update your PR and I'll merge it?

@nouiz
Copy link
Contributor Author

nouiz commented Oct 31, 2023

Yes, I think bumping the min version is better. Could you please update your PR and I'll merge it?

I amended the commit. Thanks.

@fineguy fineguy added the copybara-import Internal label for PR management label Nov 1, 2023
@copybara-service copybara-service bot merged commit e82921d into tensorflow:master Nov 1, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
copybara-import Internal label for PR management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants