Page MenuHomeSoftware Heritage

Added pyblake2 in py3 test dependency
Needs ReviewPublic

Authored by twitu on Wed, Jun 12, 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
Branch
pyblake-dependency
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6292
Build 8709: tox-on-jenkinsJenkins
Build 8708: arc lint + arc unit

Event Timeline

twitu created this revision.Wed, Jun 12, 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.Wed, Jun 12, 5:25 PM
  • moved dependency to requirements-test.txt
twitu added a comment.EditedWed, Jun 12, 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.EditedThu, Jun 13, 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.Thu, Jun 13, 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.EditedThu, Jun 13, 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.Tue, Jun 18, 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.Tue, Jun 18, 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.Tue, Jun 18, 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.Tue, Jun 18, 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.Tue, Jun 18, 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.Tue, Jun 18, 2:53 PM
twitu added a comment.Tue, Jun 18, 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.Tue, Jun 18, 3:08 PM
  • Move platform specific dependency to install_requires
olasd accepted this revision.Tue, Jun 18, 3:11 PM

Thanks!

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

Add pyblake2 dependency for python<3.6

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

Foolishly committed to master earlier, created new branch now