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

Testing using notebook>=6.1 requires requests_unixsocket #5644

Closed
fcollonval opened this issue Aug 1, 2020 · 10 comments · Fixed by #5650
Closed

Testing using notebook>=6.1 requires requests_unixsocket #5644

fcollonval opened this issue Aug 1, 2020 · 10 comments · Fixed by #5650

Comments

@fcollonval
Copy link
Collaborator

fcollonval commented Aug 1, 2020

Update to provide a quick answer:

If you are relying on notebook.tests for testing your own code (like a jupyter server extension), be sure to install notebook[test] dependencies in your test environment. Starting with notebook>=6.1, requests-unixsocket was added as test dependency.


That import makes notebook.test incompatible with Windows:

import requests_unixsocket

Is it even used as that package was removed in #5451?

If it is needed what about using pytest.importorskip ?

@kevin-bates
Copy link
Member

Well, that didn't take long! 😄

@fcollonval - thank you for opening this issue.

Could you please elaborate on how the import of requests_unixsocket makes notebook.test incompatible with Windows?

requests_unixsockets is used to emulate unix socket behavior in the tests and, yes, those particular tests are skipped on Windows. However, the pip install .[test] statement in the Windows tests in appveyor is successfully installing (and importing) requests_unixsocket.

Successfully installed Send2Trash-1.5.0 argon2-cffi-20.1.0 atomicwrites-1.4.0 coverage-5.2.1 iniconfig-1.0.1 more-itertools-8.4.0 nbval-0.9.6 nose-exclude-0.5.0 nose-warnings-filters-0.1.5 notebook-7.0.0.dev0 pluggy-0.13.1 prometheus-client-0.8.0 py-1.9.0 pytest-6.0.1 pytest-cov-2.10.0 pywinpty-0.5.7 requests-unixsocket-0.2.0 selenium-3.141.0 terminado-0.8.3 toml-0.10.1

It is true that this particular package was added to test dependencies in 6.1 so I can understand an existing environment encountering an import error if the test dependencies were not updated.

I agree that this could (and probably should) be tightened up a bit, making it a platform-specific dependency and import statement, but I'm trying to understand how this is an incompatibility issue and want to be sure it isn't truly blocking you.

@fcollonval
Copy link
Collaborator Author

@kevin-bates Thanks for the quick reply. Indeed I should have be more precise on the context. The notebook.tests.launchnotebook module is quite useful to test extension handlers; I was in particular working on jupyter_conda. And got the error after updating my environment.

But you are right, the package is pure Python (despite addressing pure Unix feature). So the problem is more about the additional dependency for testing of extension packages. So I see two solutions:

  • Use a better issue title and highlight the need to add requests_unixsockets in all test environments of packages using notebook.tests.launchnotebook (so people hitting that trouble got a quick answer from this issue). And somehow advertise about this change within the community.
  • Move the import locally to the only place it is used - this reduce the burden for extension authors. But implies a patch release from your side.

@kevin-bates
Copy link
Member

Thanks for the clarification @fcollonval. I think the right thing to do is better-isolate the dependency and produce a patch.

That said, I do feel that those dependent on notebook's test framework should be aware that dependencies can change (at any time) and the onus is on them to ensure those requirements are satisfied by using pip install --upgrade notebook[test] as a prerequisite.

@fcollonval fcollonval changed the title [Nb 6.1] Using requests_unixsocket makes notebook.test incompatible with Windows Testing using notebook>=6.1 requires requests_unixsocket Aug 1, 2020
@fcollonval
Copy link
Collaborator Author

I think the right thing to do is better-isolate the dependency and produce a patch.

Thank you for easing my life 😉

That said, I do feel that those dependent on notebook's test framework should be aware that dependencies can change (at any time) and the onus is on them to ensure those requirements are satisfied by using pip install --upgrade notebook[test] as a prerequisite.

You are right - but that is of course more complicated when relying on conda...

@bollwyvl
Copy link
Contributor

bollwyvl commented Aug 1, 2020 via email

@fcollonval
Copy link
Collaborator Author

Thanks for your input @bollwyvl

On the conda front, unfortunately the testing is light (testing import notebook and cli help).

It may be interesting to move to a multiple outputs package. But is it worth it knowing jupyter_server is the future?

@bollwyvl
Copy link
Contributor

bollwyvl commented Aug 1, 2020

As I said, doing these changes here would be a lot of work, and may not be worth it. And sure, jupyter_server is coming. However, use of (and therefore testing with) notebook will continue for some time, even within first-party Jupyter offerings, for at least a year, if not longer. It will remain in use by downstreams well beyond that. For example, even if no new notebook releases were forthcoming, python 3.9 is coming, and we'll need a new version packaged for all 6 platforms: something will break, somewhere (read: windows). The 3.8 rollout was disasterous, and took almost six months from the release of 3.8.0 to fully resolve.

With the addition of even more supported platforms that many upstreams aren't even thinking about, the only feasible way to do this is by being as aggressive as possible with automated testing: pip check, PRing dependency feedstocks when things break, using coverage thresholds, etc. I usually draw the line at browser tests, though. Worst case: every couple weeks, I have to fix something, or add a test skip for something that isn't marked appropriately or has expectations like being in a git checkout. Best case: we find upstream issues, coming in with a full reproduction case, if not a PR, which is about the only way I've found to garner good will as a downstream packager.

On conda-forge: the official line, of course, is "conda-forge is not your CI". My personal stance, for the good of the brand, is to never knowingly push out versions that cause breakage on user systems, as this results in unhappy users, and issues created on already-maintainer-strapped projects who may not even like conda to begin with. I'd far rather just not ship a version, get a patch upstream, and wait for the next release.

@kevin-bates
Copy link
Member

Thanks @bollwyvl - I agree that this repo will require a lengthy maintenance and yes, we need to prevent disruptions for downstream applications - even if only related to testing. We should try to note any dependency changes and understand their downstream impacts. (I probably should have made that connection for 6.1, but this is a rookie at helm.)

Given the diminishing maintainer focus on this repo, I think it is best to take the easiest route for this particular issue and avoid installing requests_unixsockets on Windows. This does not preclude making a concerted effort to shore up the packaging at another time though.

@bollwyvl
Copy link
Contributor

bollwyvl commented Aug 3, 2020 via email

@fcollonval
Copy link
Collaborator Author

Indeed thank you all.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants