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

Try to switch to h5py 3.1.0 #45380

Merged
merged 20 commits into from Dec 7, 2020
Merged

Try to switch to h5py 3.1.0 #45380

merged 20 commits into from Dec 7, 2020

Conversation

bhack
Copy link
Contributor

@bhack bhack commented Dec 3, 2020

No description provided.

@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Dec 3, 2020
@google-cla google-cla bot added the cla: yes label Dec 3, 2020
@bhack
Copy link
Contributor Author

bhack commented Dec 3, 2020

This is the new strings r/w behavior for 3.x series: https://docs.h5py.org/en/latest/strings.html

@bhack
Copy link
Contributor Author

bhack commented Dec 3, 2020

Just to reference the original ticket #44467

@gbaned gbaned self-assigned this Dec 4, 2020
@gbaned gbaned added comp:keras Keras related issues prtype:bugfix PR to fix a bug labels Dec 4, 2020
@kkimdev kkimdev requested review from mihaimaruseac and removed request for rohan100jain, kkimdev and qqfish December 4, 2020 04:02
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Dec 4, 2020
@bhack
Copy link
Contributor Author

bhack commented Dec 4, 2020

In the CI logs I see different unicode issues related to https://docs.h5py.org/en/latest/strings.html#what-about-numpy-s-u-type

TypeError: No conversion path for dtype: dtype

But CPU tests don't fail locally inside the official TF devel image tensorflow/tensorflow:devel.

It is hard to work locally where official devel images are not aligned with the CI.
I think an user can expect to collect the same test result of the CI Ubuntu CPU as locally with tensorflow/tensorflow:devel if not it will be hard to develop in a local reproducible system.

I think that probably this is caused unaligned tensorflow/tensorflow:devel with LANG_CTYPE=C.UTF-8 and other image used from the CI that are LC_CTYPE="POSIX"

@bhack
Copy link
Contributor Author

bhack commented Dec 4, 2020

I've tried to use POSIX locally in tensorflow/tensorflow:devel but tests are passing. I cannot reproduce CI fails..

So I've just reverted on one of the first commit to let you see that Linux is passing but not Win/Mac

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Dec 5, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 5, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 5, 2020
@copybara-service copybara-service bot merged commit e37cdb6 into tensorflow:master Dec 7, 2020
PR Queue automation moved this from Approved by Reviewer to Merged Dec 7, 2020
@bhack
Copy link
Contributor Author

bhack commented Dec 7, 2020

@mihaimaruseac What was the Win and OSX issue?

@mihaimaruseac
Copy link
Collaborator

Windows seems to be broken at HEAD due to an MLIR integrate from 12 hours ago.

MacOS seemed to pass. Probably the environment is different? Hopefully there is no rollback.

copybara-service bot pushed a commit that referenced this pull request Dec 8, 2020
PiperOrigin-RevId: 346215317
Change-Id: Id990b67a0e3c446555b590c0711159bd3ab88465
@bhack
Copy link
Contributor Author

bhack commented Dec 8, 2020

@mihaimaruseac Can you reopen this? Do you have Shell access on a win/MacOS machine?

@mihaimaruseac
Copy link
Collaborator

I cannot reopen, GitHub does not let me :(

I can get access to a windows machine, but it will take a while as I have a few P0 items to finish this week/early next week.

bhack added a commit to bhack/tensorflow that referenced this pull request Dec 8, 2020
@bhack
Copy link
Contributor Author

bhack commented Dec 8, 2020

Yes I know it is merged but I meant something like #45487

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:keras Keras related issues prtype:bugfix PR to fix a bug ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants