Page MenuHomeSoftware Heritage

Added pyblake2 in py3 test dependency
ClosedPublic

Authored by twitu on Jun 12 2019, 5:03 PM.

Details

Summary

Command: tox

Expected behaviour: Test completion

Observed behaviour: Test fails due to import error on pyblake2

Diff Detail

Repository
rDMOD Data Model
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

twitu created this revision.Jun 12 2019, 5:03 PM
ardumont added inline comments.
tox.ini
12 ↗(On Diff #5201)

If it's for the tests, that should go within the requirements-text.txt

twitu updated this revision to Diff 5202.Jun 12 2019, 5:25 PM
  • moved dependency to requirements-test.txt
twitu added a comment.EditedJun 12 2019, 6:28 PM

https://forge.softwareheritage.org/P429
this is the error message from one of the tests. Results in all py3 tests failing.

I'm surprised since [1] is currently (without the diff's content i mean) green.
It has been for a while ;)

[1] https://jenkins.softwareheritage.org/job/DLDG/job/tests/

Ok, so like i said in irc, pyblake2 is a transitive dependency of the loader-git through the swh-model (in charge of computing hashes).
So we should not have to declare that here.

So the problem you are facing must lie elsewhere.

Have you tried to recreate the tox environment (prior to this commit)?

tox --recreate
twitu added a comment.EditedJun 13 2019, 10:48 AM

I was clearing existing tox setup with rm -rf .tox, when I changed requirements. I will try tests of other modules as well and report if I find similar errors.

twitu added a comment.Jun 13 2019, 1:11 PM
.tox/py3/lib/python3.5/site-packages/swh/model/hashutil.py:211: in _new_hashlib_hash
    return _new_blake2_hash(algo)

def _new_blake2_hash(algo):
        """Return a function that initializes a blake2 hash.
    
        """
    
        < -- removed for clarity -->
    
        if lalgo in hashlib.algorithms_available:
            # Handle the case where OpenSSL ships the given algorithm
            # (e.g. Python 3.5 on Debian 9 stretch)
            _blake2_hash_cache[algo] = lambda: hashlib.new(lalgo)
        else:
            # Try using the built-in implementation for Python 3.6+
            if blake_family in hashlib.algorithms_available:
                blake2 = getattr(hashlib, blake_family)
            else:
>               import pyblake2
E               ImportError: No module named 'pyblake2'

As you can see I am using python3.5 which is the default version installed as per instructions in developer setup. But the code assumes Python 3.6+ when it does not find lalgo. I am not sure why it is working everywhere but not on my setup.

.tox/py3/lib/python3.5/site-packages/swh/model/hashutil.py:211: in _new_hashlib_hash
    return _new_blake2_hash(algo)
def _new_blake2_hash(algo):
        """Return a function that initializes a blake2 hash.
        """
        < -- removed for clarity -->
        if lalgo in hashlib.algorithms_available:
            # Handle the case where OpenSSL ships the given algorithm
            # (e.g. Python 3.5 on Debian 9 stretch)
            _blake2_hash_cache[algo] = lambda: hashlib.new(lalgo)
        else:
            # Try using the built-in implementation for Python 3.6+
            if blake_family in hashlib.algorithms_available:
                blake2 = getattr(hashlib, blake_family)
            else:
>               import pyblake2
E               ImportError: No module named 'pyblake2'

As you can see I am using python3.5 which is the default version installed as per instructions in developer setup. But the code assumes Python 3.6+ when it does not find lalgo. I am not sure why it is working everywhere but not on my setup.

It seems like there is something fishy indeed.

What distribution are you using (for understanding and reproductibility concerns)?


In any case, the problem lies in swh-model repository though.
We can finish discussing it here first though, we'll reference back if we need to.

twitu added a comment.EditedJun 13 2019, 4:45 PM

I am on a standard Ubuntu 16.04 Xenial distribution. Is there any specific config files you want to see.

I am on a standard Ubuntu 16.04 Xenial distribution. Is there any specific config files you want to see.

Thanks.

In this context, come to think of it (more), that should be irrelevant... Since we are using tox (thus pip and pypi underneath)...

In D1574#35522, @twitu wrote:

I am not sure why it is working everywhere but not on my setup.

I am guessing this is because we use Debian 9 everywhere, whose openssl supports blake2s256.

I don't want to look for that information later so here is the excerpt from @olasd on irc (which sounds totally legit, though i did not check further than that):

14:00 <+olasd> so, twitu[m]'s issue is that : swh.loader.git depends on swh.model; swh.model has been uploaded as a platform-independent wheel to PyPI; swh.model has setup.py magic to detect whether pyblake2 should be installed; we built the wheel on a platform that doesn't need pyblake2 to be installed; installing a wheel doesn't run setup.py
14:01 <+olasd> (but uses requires information from the build platform for dependency resolution)

I don't know yet how to fix properly the issue (again the fix is due in swh-model anyway).

vlorentz requested changes to this revision.Jun 18 2019, 10:14 AM

A middle-ground would be to change swh-model to install pyblake2 on all systems with a Python < 3.6, by using this in the dependency declaration: extras_require={':python_version<"3.6"':: ['pyblake2']}

This will install pyblake2 even on systems that ship an openssl with support for blake2, but at least it won't break.

This revision now requires changes to proceed.Jun 18 2019, 10:14 AM

A middle-ground would be to change swh-model to install pyblake2 on all systems with a Python < 3.6, by using this in the dependency declaration: extras_require={':python_version<"3.6"':: ['pyblake2']}

That sounds completely reasonable to me.
Thanks for the idea.

twitu added a comment.Jun 18 2019, 1:19 PM
	blake2_requirements = []
	
	pyblake2_hash_sets = [
	    # Built-in implementation in Python 3.6+
	    {'blake2s', 'blake2b'},
	    # Potentially shipped by OpenSSL 1.1 (e.g. Python 3.5 in Debian stretch
	    # has these)
	    {'blake2s256', 'blake2b512'},
	]
	
	for pyblake2_hashes in pyblake2_hash_sets:
	    if not pyblake2_hashes - set(hashlib.algorithms_available):
	        # The required blake2 hashes have been found
	        break
	else:
	    # None of the possible sets of blake2 hashes are available.
	    # use pyblake2 instead
	    blake2_requirements.append('pyblake2')

This snippet from setup.py in swh-model adds pyblake2 to blake2_requirements in my local setup. Shouldn't be already installed then?

The setup.py is not run when you install a wheel. It's pre-computed by the computer that compiles the wheel, not yours.

twitu updated this revision to Diff 5299.Jun 18 2019, 2:21 PM

Add pyblake2 dependency for all python versions below 3.6

Remove extra code because hashlib checking is no longer required

olasd requested changes to this revision.Jun 18 2019, 2:53 PM

I don't think that extras_require is going to work.

According to the distutils documentation, platform specific requirements need to be registered in the install_requires key.

There's the extra caveat that platform-specific requires are not supported by pip in the requirements.txt files, and therefore we need to encode the pyblake2 dependency directly in setup.py (sigh).

In the end, the proper solution is to replace the blake2_requirements block with

blake2_requirements = ['pyblake2;python_version<"3.6"']

and leave the rest untouched.

This revision now requires changes to proceed.Jun 18 2019, 2:53 PM
twitu added a comment.Jun 18 2019, 2:59 PM

I built and tested the wheel locally, it did install pyblake2 as per the requirements.

@twitu The issue is when the wheel is built on a computer where openssl supports blake2 and installed on a computer where openssl does not support it.

twitu updated this revision to Diff 5303.Jun 18 2019, 3:08 PM
  • Move platform specific dependency to install_requires
olasd accepted this revision.Jun 18 2019, 3:11 PM

Thanks!

twitu updated this revision to Diff 5311.Jun 18 2019, 3:51 PM

Add pyblake2 dependency for python<3.6

twitu added a comment.Jun 18 2019, 3:51 PM

Foolishly committed to master earlier, created new branch now

Thanks for the fix!

Foolishly committed to master earlier, created new branch now

That's not foolish.
And that's not an issue either.
In the end, it will land in the repository's default branch (mostly master).

My workflow is mostly to work on the master branch and diff against it.
Then i rebase my local code when the repository on that branch progresses.
And then i update the diff (prior to ci's build ok and push).

When i work on a branch, i open a diff against that branch.
In the end, when the diff is accepted.
I update the master branch with the latest modification from the team.
I merge (as a fast-forward) my local branch to master.
Then i still update the diff, wait for the ci to be ok, then i push.

That has the benefit to close automatically the diff in question.

Cheers,

twitu updated this revision to Diff 5349.Jun 19 2019, 1:32 PM

Merge branch with master

vlorentz accepted this revision.Jun 20 2019, 10:09 AM
This revision is now accepted and ready to land.Jun 20 2019, 10:09 AM
This revision was automatically updated to reflect the committed changes.