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

Support LZMA on Python 2 via backports.lzma #13

Merged
merged 3 commits into from
Feb 22, 2017
Merged

Support LZMA on Python 2 via backports.lzma #13

merged 3 commits into from
Feb 22, 2017

Conversation

jakirkham
Copy link
Member

Fixes https://github.com/alimanfoo/numcodecs/issues/11

Tries to use backports.lzma as a fallback. Adds testing for it with Travis CI.

@jakirkham jakirkham changed the title Support LZMA on Python 2 via backports.lzma WIP: Support LZMA on Python 2 via backports.lzma Feb 19, 2017
@alimanfoo
Copy link
Member

Thanks @jakirkham. I'm looking at the travis logs and I don't think backports.lzma is getting installed, which then means the tests for the LZMA codec are not being run. I don't know why, I'm guessing something to do with the "if" condition. Is there not a travis environment variable to check which is current major Python version?

@jakirkham
Copy link
Member Author

Yeah, I had some ' that caused me some issues. Have corrected. Looks like that issue is fixed now.

Is there not a travis environment variable to check which is current major Python version?

I looked for one briefly in the docs, but didn't see one. 😞 If you find something like that, let me know and I'll happily change it.

@jakirkham
Copy link
Member Author

Getting back to the matter at hand, it seems that check in the codec's config is represented as a long in Python 2, but the rest are int. In Python 3, I'm guessing these are all long. We might need to tweak some things with storing config values and encoding/decoding to JSON to support Python 2/3.

@alimanfoo
Copy link
Member

Re detecting travis python version, I think there's a $TRAVIS_PYTHON_VERSION variable, see this example.

@alimanfoo
Copy link
Member

Something I don't understand: The Python 2.7 build on travis is failing, because apparently the LZMA codec is not available, which suggests that backports.lzma was not installed successfully. However, if I look at the travis log, there is a section where it claims it is installed successfully. But then when I try to install backports.lzma on my local system, it cannot compile because lzma.h is not found, because I haven't installed XZ utils.

Could you try adding:

- if [[ $TRAVIS_PYTHON_VERSION == '2.7' ]]; then pip install -U backports.lzma; fi

...into the travis.yml file to see if gets installed successfully outside of tox?

@alimanfoo
Copy link
Member

I think I see why the PY27 travis tests are failing. The name LZMA needs to get conditionally imported up into the root numcodecs namespace. Some modification is needed to this section of code.

@alimanfoo
Copy link
Member

How does the int/long issue come up? The following works for me on PY27:

(spike-lzma) aliman@uluru:~$ python
Python 2.7.12+ (default, Sep 17 2016, 12:08:02) 
[GCC 6.2.0 20160914] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from numcodecs.lzma import LZMA
>>> repr(eval('LZMA(format=1, check=0, preset=1, filters=None)'))
'LZMA(format=1, check=0, preset=1, filters=None)'

@jakirkham
Copy link
Member Author

Thanks for all the tips.

I did recall there was an environment variable for Python on Travis CI. Thank for figuring out what it was.

We can try moving the install back outside tox. However, when I had fixed the syntax errors I found that it wasn't able to import it unless it was installed in the environment that we were using for testing. 😕 Then again I don't use tox very often. So I may just be missing something.

Good point on the LZMA codec registration code. Was familiarizing myself with that the other day. Changing that should be straightforward.

I'll take a look at the long/int issues again. IIRC the issue came up on this line. I'm not sure why, but I think _lzma.CHECK_NONE was coming up as a long.

@alimanfoo
Copy link
Member

alimanfoo commented Feb 21, 2017 via email

@jakirkham jakirkham changed the title WIP: Support LZMA on Python 2 via backports.lzma Support LZMA on Python 2 via backports.lzma Feb 21, 2017
@jakirkham
Copy link
Member Author

Well, I think registering the LZMA codec on Python 2 was all that was still needed. This seems to be installing ok and passing the tests.

Copy link
Member

@alimanfoo alimanfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks, one small comment, otherwise looks good to go.

@@ -45,4 +54,7 @@ def test_config():
check_config(codec)

def test_repr():
check_repr('LZMA(format=1, check=0, preset=1, filters=None)')
if _py2:
check_repr('LZMA(format=1, check=0L, preset=1, filters=None)')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this conditional logic needed for PY2? On my local system check_repr('LZMA(format=1, check=0, preset=1, filters=None)') works ok for both PY2 and PY3.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right. Was running into this issue before updating the registration of LZMA. Now seems ok. So have reverted this change.


from numcodecs.compat import PY2
_py2 = (sys.version_info[0] == 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can possibly remove this, see comment on test_repr.

@jakirkham
Copy link
Member Author

Have addressed your comments. Please let me know if there is anything else.

@alimanfoo alimanfoo merged commit ad7e983 into zarr-developers:master Feb 22, 2017
@alimanfoo
Copy link
Member

Thank you, very nice compatibility add.

@alimanfoo alimanfoo added this to the 0.1 milestone Feb 22, 2017
@jakirkham
Copy link
Member Author

Of course.

@jakirkham
Copy link
Member Author

Proposing a backport(?) of this to Zarr in PR ( https://github.com/alimanfoo/zarr/pull/123 ).

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.

2 participants