Asynchronous Contents API #324
Conversation
mwakaba2
force-pushed
the
async-contents-api
branch
from
a2404d8
to
ed767bc
Compare
|
Thanks @mwakaba2 - this is a good start.
You're right, most tests leverage the |
|
@kevin-bates Since Another option would be to create a no-op or simple custom ContentsManager class that inherits from the async classes, and test that. To mimic network IO, the async methods can include |
|
Hi Mariko - I think taking the forward-moving branch by implementing |
|
Hi @kevin-bates, I added the |
tests/services/contents/test_api.py
Outdated
| @pytest.fixture(params=["FileContentsManager", "AsyncFileContentsManager"]) | ||
| def argv(request): | ||
| if request.param == "AsyncFileContentsManager" and sys.version_info < (3, 6): | ||
| pytest.skip("Kernel manager is AsyncFileContentsManager, Python version < 3.6") |
There was a problem hiding this comment.
Copy/paste - Kernel -> Contents. Actually, since we no longer support 3.5, this condition can be dropped altogether. 😄
|
Hi @mwakaba2 - thank you for this great update. I hope to be able to take this for spin next week - I hope that's okay. Things look real good via a quick scan - just had the one immediate comment (to show I did look at things 😄 ). Also, we've made some fairly large changes to the pytest landscape. This will manifest in your additional tests in the form for requiring Thanks for the links to |
|
Hi @kevin-bates, hope you had a good weekend. Yeah that works! I see some code scanning lint errors, so I'll fix those along with removing the python 3.5 pytest skip logic, rebasing with master to include public fixture changes, and testing the new classes with my team's custom contentsmanager. Thanks for the quick review! |
mwakaba2
force-pushed
the
async-contents-api
branch
2 times, most recently
from
8e4065c
to
9aa57a7
Compare
|
Hi @kevin-bates, is it possible to publish a dev version to PyPI to test this branch? |
|
Hi @mwakaba2 - I'm not sure there's precedent for this and reading up on publishing dev packages to pypi (as I'm not familiar with that particular aspect) I'm finding it not a recommended practice. What I've done in the past is to use pip with a reference to the GH repo@branch, like so: Would that be sufficient? Btw, I brought up your PR in the today's server meeting and will try to spend time with this. I apologize for the delay. |
|
Ah ok no problem. Yeah, we're using an in-house tool to manage 3rd party python dependencies so I'm not able to use pip directly, but I think I found a way to test it. |
Codecov Report
@@ Coverage Diff @@
## master #324 +/- ##
==========================================
- Coverage 67.37% 65.74% -1.64%
==========================================
Files 55 56 +1
Lines 5361 6025 +664
Branches 696 804 +108
==========================================
+ Hits 3612 3961 +349
- Misses 1545 1832 +287
- Partials 204 232 +28
Continue to review full report at Codecov.
|
mwakaba2
force-pushed
the
async-contents-api
branch
from
01a8664
to
e63ccc0
Compare
|
Hi Mariko - I was just going to post that I was able to take I found that the current implementation didn't exhibit the blocking behavior that I experienced with kernel startup before we implemented async/await support - which surprised me. I also found that it seems like the current implementation was slightly better in that you could see a "clear winner" amongst the 3 requests (upwards of 50% faster), whereas, with the async implementation, the times tended to be the same across the 3 requests. I suspect this is because of "too much cooperation" since each Since the primary purpose of this is to address high-latent filesystems, I suspect there will be some "wins" there. I also wonder if we should take a close look at where I'm curious if you see similar behavior. Thank you for working on this. |
|
Hi Mariko, would you mind splitting the PEP440 update out in a separate PR? This is something I had on my plate to do today (along with an update to RELEASE.md to update the dev version following the release tagging). I think only the If you'd rather I do that, that's fine also. |
|
😄 Apparently someone also had this on their plate yesterday: 8763b5b I'll go ahead and submit a PR to add the dot and update RELEASE.md. |
You may remember that we did this sort of thing in https://github.com/jupyter/jupyter_core/blob/master/jupyter_core/version.py |
|
Thanks @jasongrout - I just saw your comment (after submitting #348). I hadn't forgotten about your change in core, but personally found that to be overly complex. That said, if others prefer that approach, I'm okay with adopting it. |
It could have been way more complex for full pep440 compliance, but we dialed it back :). The purpose for the extra stuff in that file is to provide a good versioninfo tuple that mirrors the versioninfo tuple from Python more faithfully, and to make it so that you don't have to think about how pep440 formats version numbers (dot? consistent abbreviation?). I think it probably does that as simply as possible, but (a) if there is something simpler, I'd love to see it - I think it's a bit much to stomach as well, and (b) of course, if you don't want those two goals, that's a different story. |
|
Hi @kevin-bates, thanks for testing this out! I have some bugs to fix, so I think I need a little more time to test this out, but yep I agree. There are definitely some As for the pep440 update, I made that quick change just to see if the tests are still working in this branch. I'll remove the pep440 commit and update this branch once your PR is merged to the main branch. I'm taking next week off, so I'll be able to resume testing the week after that (11/30). I noticed you're taking time off in December and was wondering if another jupyter member can help with the code review/release? |
|
Thanks @mwakaba2 - that all sounds great. Have a great week off! Although I'll be out for (essentially) December, my plan is to be around for this kind of stuff. @Zsailer can you help review this and move this forward as well. Perhaps we can target the Server Team meeting of December 3 to sync up with things? |
|
I just realized you had asked to see the test scripts. I've included them in the attached tar file but did not include the large content file. The README describes how to set these up and how to construct the larger payload. As I said, they are crude and minimally viable. 😄 |
mwakaba2
force-pushed
the
async-contents-api
branch
from
aa0f852
to
c35b04a
Compare
|
@kevin-bates Thanks for putting together the test bundle.
Here are the results. I confirmed that there's no noticeable performance difference between "Before" and "After (FileContentsManager)", so that's good. 😀 So I removed some await statements to reduce the PUT response time. The increase is minimal now. This is just an update for PUT, so I'll look into optimizations for the other file operations next (e.g. rename, delete). |
|
Hi @kevin-bates and @Zsailer, the PR is ready for review. I performed three tests:
|
There was a problem hiding this comment.
This is a great contribution @mwakaba2. As Jupyter expands across more filesystem types and concurrency demands increase, I believe the async contents you've provided will prove useful - thank you!
Zsailer
force-pushed
the
async-contents-api
branch
from
5033aa8
to
b87d446
Compare
Zsailer
force-pushed
the
async-contents-api
branch
from
b87d446
to
7d863f3
Compare
|
Thanks, @mwakaba2! Looks great! Merging away! 🎉 |
Asynchronous Contents API
Description
Hi, here's the draft for adding an async option for the contents api.
So far, I added an async option for each class/mixin below:
ContentsManagerandAsyncContentsManagerCheckpointsandAsyncCheckpointsGenericCheckpointsMixinandAsyncGenericCheckpointsMixin@kevin-bates and/or @Zsailer I'd appreciate your review and feedback!
Open Questions
I added a test for the configuration, but how do we want to test the new classes since most methods are not implemented and left up to the user?
Checklist
AsyncFileContentsManagerFileContentsManagerandAsyncFileContentsManagerContentsManager