-
Notifications
You must be signed in to change notification settings - Fork 67
Writing LZ4 compressed ROOT files #288
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked everything, including the xxhash installation message. It's great!
@@ -143,7 +143,7 @@ def get_description(): | |||
test_suite = "tests", | |||
install_requires = ["numpy>=1.13.1", "awkward>=0.10.0", "uproot-methods>=0.6.0", "cachetools"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't xxhash
appear here rather than tests_require
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For pip, which is determined by setup.py, lz4 and therefore xxhash are optional dependencies, though we want to test them. For conda, they're strict dependencies, and that's controlled by uproot-feedstock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, well then this import should be wrapped by a try:
statement if it is optional. I encountered an error because I didn't have xxhash
installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it absolutely must. @reikdas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it should be inside of the function call, just like import lz4
is, and probably right after import lz4
to indicate the relationship. Although I think xxhash
is used in a different function... anyway, it should be inside of a function so that it doesn't load on startup if you have it but don't use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this was my mistake. I opened a PR fixing this. Also @jpivarski I have a draft release ready once you have merged it.
Fixes #279