-
Notifications
You must be signed in to change notification settings - Fork 150
Added base Conda environment yml file #286
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #286 +/- ##
==========================================
+ Coverage 94.69% 94.74% +0.05%
==========================================
Files 13 13
Lines 1620 1638 +18
==========================================
+ Hits 1534 1552 +18
Misses 86 86
Continue to review full report at Codecov.
|
CJ-Wright
left a comment
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.
Thank you! This looks good, should we include the doc building things too?
|
I think more docs is always great! Specifically which doc building things are you referring? I can add them pretty quickly if you can point me to them. |
|
Ah, yes I misunderstood what you were talking about. I will add those now. |
conda/environments/streamz_dev.yml
Outdated
| - defaults | ||
| dependencies: | ||
| - python>=3.7 | ||
| - cython>=0.29,<0.30 |
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.
Where is the cython dep coming from?
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.
cytoolz? I don't suppose we're building anything.
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.
Right, if one of the deps needs it conda should figure that out.
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.
ahh ... my bad I meant to remove that. I was experimenting with implementing librdkafka with cython and forgot to remove it. I will remove them now.
On a side note that is working very well BTW and might open something up around that as well. Working on the best way to make cython not a hard dependency at the moment since I'm sure several people don't using streamz don't necessarily want 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.
Working on the best way to make cython not a hard dependency
Typically you can include the C file for those that want to compile (source dist) and of course, if you build wheels and conda packages, those should not need cython.
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'm confused, do you mean that you are using librdkafka with cython or using cython to speed up streamz kafka bindings?
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'm confused, do you mean that you are using librdkafka with cython or using cython to speed up
streamzkafka bindings?
Actually both. I was replacing the current streamz kafka sources/bindsings with my own sources (AKA: not the confluent-python-kafka library) which were cython modules that interacted directly with librdkafka++. There are many advantages to that especially in the realm of partition balancing that the new KafkaConsumer offers (not implemented in the confluent python library yet) for distributed systems with many processes appearing/disappearing. I actually saw this #233 while messing around with it and think it could be helpful here as well.
In a nutshell I was seeing about 15-20X speedups in my consumers when using my cython implementations.
Actually this one as well would benefit #279
|
@martindurant thoughts? |
|
I'm generally OK with the idea, but wonder whether the CI tests should explicitly use the same environment. |
|
@jdye64 would you be interested in changing the |
|
Sure. I noticed there are a few other dependencies in the .travis.yml file that were being installed so I'm going to add those to the environment.yml file as well. |
… manually installing them. Also added a few more dependencies to the environment file that were missed
|
@CJ-Wright Ok had to step away for a bit but looks like its all good to go and using the conda environment file correctly in Travis now. |
|
Thank you very much! |
No description provided.