Skip to content

Conversation

@jdye64
Copy link
Contributor

@jdye64 jdye64 commented Dec 3, 2019

No description provided.

@jdye64 jdye64 mentioned this pull request Dec 3, 2019
@codecov
Copy link

codecov bot commented Dec 3, 2019

Codecov Report

Merging #286 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
streamz/sources.py 95.36% <0%> (+0.06%) ⬆️
streamz/dataframe/core.py 91.84% <0%> (+0.23%) ⬆️
streamz/utils_test.py 87.77% <0%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4f2a27...1fa1fb4. Read the comment docs.

Copy link
Member

@CJ-Wright CJ-Wright left a 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?

@jdye64
Copy link
Contributor Author

jdye64 commented Dec 3, 2019

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.

@CJ-Wright
Copy link
Member

@jdye64
Copy link
Contributor Author

jdye64 commented Dec 3, 2019

Ah, yes I misunderstood what you were talking about. I will add those now.

- defaults
dependencies:
- python>=3.7
- cython>=0.29,<0.30
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

@jdye64 jdye64 Dec 3, 2019

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?

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

@CJ-Wright
Copy link
Member

@martindurant thoughts?

@martindurant
Copy link
Member

I'm generally OK with the idea, but wonder whether the CI tests should explicitly use the same environment.

@CJ-Wright
Copy link
Member

@jdye64 would you be interested in changing the .travis.yaml to use the environment.yml file to build the testing env?

@jdye64
Copy link
Contributor Author

jdye64 commented Dec 4, 2019

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
@jdye64
Copy link
Contributor Author

jdye64 commented Dec 4, 2019

@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.

@CJ-Wright
Copy link
Member

Thank you very much!

@CJ-Wright CJ-Wright merged commit 8dfd65a into python-streamz:master Dec 5, 2019
@jdye64 jdye64 deleted the 285 branch December 9, 2019 00:48
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.

3 participants