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

Rebuild for hdf51106 #75

Conversation

regro-cf-autotick-bot
Copy link
Contributor

This PR has been triggered in an effort to update hdf51106.

Notes and instructions for merging this PR:

  1. Please merge the PR only after the tests have passed.
  2. Feel free to push to the bot's branch to update this PR if needed.

Please note that if you close this PR we presume that the feedstock has been rebuilt, so if you are going to perform the rebuild yourself don't close this PR until the your rebuild has been merged.

This package has the following downstream children:

And potentially more.

If this PR was opened in error or needs to be updated please add the bot-rerun label to this PR. The bot will close this PR and schedule another one. If you do not have permissions to add this label, you can use the phrase @conda-forge-admin, please rerun bot in a PR comment to have the conda-forge-admin add it for you.

This PR was created by the regro-cf-autotick-bot.
The regro-cf-autotick-bot is a service to automatically track the dependency graph, migrate packages, and propose package version updates for conda-forge. If you would like a local version of this bot, you might consider using rever. Rever is a tool for automating software releases and forms the backbone of the bot's conda-forge PRing capability. Rever is both conda (conda install -c conda-forge rever) and pip (pip install re-ver) installable.
Finally, feel free to drop us a line if there are any issues!
This PR was generated by https://circleci.com/gh/regro/circle_worker/24268, please use this URL for debugging

regro-cf-autotick-bot added 2 commits June 16, 2020 14:06
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jakirkham jakirkham mentioned this pull request Jun 16, 2020
@jakirkham
Copy link
Member

Seeing the following failure on Windows...

================================== FAILURES ===================================
_______________ TestUnicode.test_unicode_hdf5_python_consistent _______________

self = <h5py.tests.test_file.TestUnicode testMethod=test_unicode_hdf5_python_consistent>

    def test_unicode_hdf5_python_consistent(self):
        """ Unicode filenames can be used, and seen correctly from python
        """
        fname = self.mktemp(prefix=six.unichr(0x201a))
        with File(fname, 'w') as f:
>           self.assertTrue(os.path.exists(fname))
E           AssertionError: False is not true

..\_test_env\lib\site-packages\h5py\tests\test_file.py:493: AssertionError
============================== warnings summary ===============================

More details in CI. Also full logs attached.

@kmuehlbauer
Copy link

@jakirkham Seems like the Unicode temp file isn't properly created. But how can we be sure, if that's really the case?

@likewei92
Copy link

It'd be great to get this merged since newer packages (like pytables) are locking hdf5 version to >=1.10.6 in conda now (:

@jakirkham
Copy link
Member

Help on this would be welcome. I lack a Windows machine and it's fairly rare I do development there. So this probably needs someone who has that expertise/resources to push this forward. Please let us know if you have someone (or you yourself are that someone 😉) 🙂

@scopatz
Copy link
Member

scopatz commented Jun 24, 2020

Maybe we can just skip that test if that is the only failure

@likewei92
Copy link

I have a Windows machine but I have no idea how you run your tests. If you can help me get set up I can look into this test

@jakirkham
Copy link
Member

jakirkham commented Jun 24, 2020

Great, thanks for helping out @likewei92! 😄

Should just be a matter of checking out this PR locally and running something like this...

conda-build.exe -m .ci_support\win_python3.8.____cpython.yaml recipe

There are different variant files in .ci_support for the different Python version builds. Chose Python 3.8 arbitrarily in the example above. Also this assumes you have conda-build installed in some local Conda install.

@likewei92
Copy link

I looked into this and found that the file was being created correctly but was somehow not being opened correctly. If you changed with File(fname, 'w') as f to with open(fname, 'w')as f the test works. To fix that we'd need to do a PR into h5py itself. I'm not exactly sure what the root cause is, but it's probably to do with fname not being interpreted correctly as unicode by File.

In any case, it looks like this tests is safe to ignore. Should we do that as @scopatz suggested?

@jakirkham
Copy link
Member

Thanks for exploring this @likewei92! 😄

Interesting, with that context, did a bit of searching. Looks like upstream issue ( h5py/h5py#839 ) may be related. Also there's a patch suggested for hdf5 in issue ( conda-forge/hdf5-feedstock#47 ). So maybe this is a deeper issue than just h5py.

Am ok with skipping the test. Though I guess this means there will be some issues opening files on Windows. Are we ok with that?

@likewei92
Copy link

So I looked into this a little more: the file that's created on disk (when I viewed it with file explorer) has a � in it where the unicode character is supposed to be. os.path.exists(f.filename) also returns False, which seems like a legitimate bug.

I'm ok with bypassing this test for now - I'm actually using h5py on Linux and this is blocking an environment upgrade for me. If we're concerned about the Windows bug, is it possible to publish the Linux version to conda forge first?

@jakirkham
Copy link
Member

Yeah no objection moving forward as long as we are aware of the tradeoffs. Does someone know what we would need to add to skip this test?

@likewei92
Copy link

The test itself is in hdf5 so I don't think we'd be able to tell pytest to skip if unless we modify hdf5 code. Is it possible to override the CI policy and merge this PR?

@jakirkham
Copy link
Member

It looks like the test is in h5py. It would be better to fix CI. If someone would like to suggest how we skip this test, that would be welcome.

@scopatz
Copy link
Member

scopatz commented Jun 26, 2020

h5py just uses pytest

@duncanmmacleod
Copy link
Contributor

The way that the tests are executed inside h5py is a little restrictive, but this patch skips the test and passes the build on my windows system:

From 2b11c25bc866d0fceb52b27da4f51836cbbe727c Mon Sep 17 00:00:00 2001
From: Duncan Macleod <[email protected]>
Date: Fri, 26 Jun 2020 10:22:45 +0100
Subject: [PATCH] skip failing unicode test on windows

---
 recipe/meta.yaml                       |   2 ++
 recipe/skip-failing-unicode-test.patch | Bin 0 -> 535 bytes
 2 files changed, 2 insertions(+)
 create mode 100644 recipe/skip-failing-unicode-test.patch

diff --git a/recipe/meta.yaml b/recipe/meta.yaml
index 547523e..dfb04b9 100644
--- a/recipe/meta.yaml
+++ b/recipe/meta.yaml
@@ -22,6 +22,8 @@ source:
     # see https://github.com/h5py/h5py/issues/1244
     #     https://github.com/h5py/h5py/pull/1325
     - ppc64le_test.patch
+    # skip failing unicode test on windows
+    - skip-failing-unicode-test.patch  # [win]

 build:
   number: {{ build }}
diff --git a/recipe/skip-failing-unicode-test.patch b/recipe/skip-failing-unicode-test.patch
new file mode 100644
index 0000000000000000000000000000000000000000..1175aa6a6f86420cf2243b4612b0d9b7a336b9a1
GIT binary patch
literal 535
zcmbtQ!HUBm6ukQtk6xOPnzY>}+g&L1KbA#e)JE2*sDb3~R|DBXA*c25csk?EFiZ*o
z7(Zlyw(4s;SAqNBD?+4}fsoS9KsNv+HRBV!3EEuSGb>9<>vK&%-r;mSHkS*+7;mbg
zqA0eN?{~nb>!E>SneocF05@u~#LHUu@VEUw|3t7J;miQOS22+stZ@OnRWiI1Mrxhu
zxc4ioYn6H>&3`DLcsGWYDvt2#)F}x!_<BMI*x_Lpr=3&0v|()L6f*6p*4;NM{`Eg;
z{v<VR0wfnQYS|gaNzX)0qH}LF>5TT$2d+ajT>T&hF8LH+S+;bt)R*T^g5zOy8d4Qm
HB<JWCYE`4A

literal 0
HcmV?d00001

--
2.27.0.windows.1

The content of the binary patch file should be:

diff --git a/h5py/tests/__init__.py b/h5py/tests/__init__.py
index 3fb68a2d..bb525b97 100644
--- a/h5py/tests/__init__.py
+++ b/h5py/tests/__init__.py
@@ -18,6 +18,6 @@ def run_tests(args=''):
         from shlex import split
         from subprocess import call
         from sys import executable
-        cli = [executable, "-m", "pytest", "--pyargs", "h5py"]
+        cli = [executable, "-m", "pytest", "--pyargs", "h5py", "-k", "not test_unicode_hdf5_python_consistent"]
         cli.extend(split(args))
         return call(cli)

@scopatz
Copy link
Member

scopatz commented Jun 26, 2020

Cool! Can someone please send a PR based off this branch that includes that patch?

@likewei92
Copy link

FYI you can skip the test in this repo itself (instead of h5py) by using exit(h5py.run_tests('-k "not test_unicode_hdf5_python_consistent"')) in recipe\run_test.py instead of exit(h5py.run_tests())

@duncanmmacleod
Copy link
Contributor

Cool! Can someone please send a PR based off this branch that includes that patch?

Doing it now, including @likewei92's simpler test modification.

@duncanmmacleod duncanmmacleod mentioned this pull request Jun 26, 2020
5 tasks
@kmuehlbauer
Copy link

We might even make an OS switch there, adding @likewei92's suggestion only if Windows is the running OS.

@github-actions github-actions bot closed this in #76 Jun 27, 2020
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.

7 participants