-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Add Python package sos-notebook. #7665
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
Yes, I can join as a co-maintainer. |
Yes I'm willing to be a co-maintainer. Thanks! |
@BoPeng Are you sure that the SoS kernel doesn't need matplotlib? Using the conda version of sos-notebook that I've uploaded to my personal channel:
The kernels for Python3 and R both work:
But the SoS kernel does not:
After I install matplotlib, then the SoS kernel works. |
As you can see from the error message, the function is inside But if we need |
Right. But the ipykernel works without matplotlib (at least for a simple calculation like https://github.com/conda-forge/ipykernel-feedstock/blob/master/recipe/meta.yaml#L34
We have multiple options to consider:
|
Please go with 2 and I will see what I can do about 3 later. |
bfe18b1
to
5361e38
Compare
ok, I have stopped setting matplotlib mode to Although SoS |
@conda-forge/staged-recipes This is ready for review. I'd appreciate advice on my tests to ensure that the sos kernel is properly installed. Because the name of the pacakge, sos-notebook, is in the filepath, the tests that search for "sos" in the output from
|
5361e38
to
60b54b0
Compare
ipykernel does not install it by default because it can run non-plotting code without matplotlib. However, the SoS kernel requires matplotlib to run any code.
3f0be22
to
e389f42
Compare
@BoPeng Could you please advice on how to that the SoS kernel is functional after the conda package is installed? I've tried multiple different ideas, but I keep getting errors. I tried testing the kernel using jupyter_kernel_test, but this failed with the following error:
https://ci.appveyor.com/project/conda-forge/staged-recipes/builds/23437118#L1110 So then I tried an even simpler test. Just try to run a script that only contained the line https://travis-ci.org/conda-forge/staged-recipes/builds/513133132#L1515 I've also tested sos-notebook 0.19.1 locally, and it is still failing. I'll push it here so that you can also view the results with the latest version. |
url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.tar.gz | ||
sha256: 9f3ae4371f2a9cf493c77190010e73270ddc658904618937889ce14c0ecebb09 | ||
|
||
build: |
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.
Please, delete the build.sh
and bld.bat
. After that please add in your build
section
script:
- {{ PYTHON }} -m pip install . --no-deps --ignore-installed -vv
- {{ PYTHON }} -m sos_notebook.install --sys-prefix
Otherwise, this recipe cannot be noarch
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.
@marcelotrevisani Thanks! I just pushed this change.
@jdblischak sos notebook has its own test suite but some advanced usages requires other modules to test (e.g. some language modules). I did not check the details but your test might have failed due to the use of development version of SoS, which no longer has |
@BoPeng The version of SoS was the latest version available from conda-forge (0.19.2). All the tests I've tried are purposefully simple (i.e. basic arithmetic). They are just testing if it was installed properly.
Thanks! I'll update the recipe again. |
Strange. It's working now that I specified the build steps in https://travis-ci.org/conda-forge/staged-recipes/builds/513149395#L1625 |
@jdblischak Then please do not update the recipe for sos notebook 0.19.2 because I noticed that the travis test for it failed. |
@BoPeng The simple test passed on everything except Windows: https://ci.appveyor.com/project/conda-forge/staged-recipes/builds/23461251#L1100
|
I do not know what is going on but I will find a windows machine and test. Note that I have fixed tests of sos notebook and released sos notebook 0.19.3, so if you would like to upgrade, 0.19.3 is the version to try. For testing if the basic features (namely kernel switch) of sos notebook work correctly, a basic unit test would be import unittest
from ipykernel.tests.utils import execute, wait_for_idle, assemble_output
from sos_notebook.test_utils import sos_kernel
class TestSoSKernel(unittest.TestCase):
def testKernel(self):
with sos_kernel() as kc:
execute(kc=kc, code='a = 1\n')
wait_for_idle(kc)
execute(kc=kc, code='%use Python3\n%get a\nb = a + 1')
wait_for_idle(kc)
execute(kc=kc, code='%use SoS\n%get b --from Python3\nprint(b)')
stdout, stderr = assemble_output(kc.iopub_channel)
self.assertEqual(stderr, '')
self.assertEqual(stdout.strip(), '2')
if __name__ == '__main__':
unittest.main() so you can include this file and run |
I updated the recipe to 0.19.3. Fingers crossed! |
The problem persists so this is a timing issue. Basically on a slow system the kernel will take a while to start so the test times out. Changing
to
will make the test use longer |
I can't get the sos kernel to work inside of a conda environment. What happens when you run the code below on a Windows machine?
When I run this, I get the following error (and it stalls, i.e. I have to press
And one time I got the following error message:
And to be fair, sometimes the python3 kernel also fails:
Doing some searching, there are lots of threads about Windows security features affecting Jupyter in a conda environment, e.g. this SO question |
@jdblischak I used a very tedious way to check why the test failed. Basically, I created a minimal test case for sos-notebook for a branch of sos-notebook with a minimal appveyor config . Instead of using I then compared packages installed by this setup with the one that failed, and found that the following packages are missing:
Basically, |
@BoPeng Great work! I added prompt_toolkit and prometheus_client from conda-forge. I had noticed that ptyprocess was being installed via |
Running |
Still failed. Perhaps |
Another way to test is to remove |
@BoPeng Good ideas. I'll investigate more either later today or tomorrow. |
Please just add |
It's not possible to include a pip dependency in a conda recipe. See conda/conda-build#548 The Windows build will fail early because it won't be able to find ptyprocess. I already ran this locally, which is how I originally discovered that ptyprocess was not available for Windows. |
Never mind, |
Shoot. That is sad. Thanks for investigating! |
I just added And here is a list of pip/conda packages not found in the failed one
|
@jdblischak Sorry, I spoke too fast. After I completely removed @isuruf It took us a lot of work to figure out why our test failed. Basically, Correction: |
@jdblischak My suggestion is that we add
to the recipe and move on since this is an upstream problem that we cannot fix. |
@BoPeng This is a disappointing end after all this troubleshooting, but I agree that without ptyprocess it won't be possible to use the SoS kernel on Windows. I did some more troubleshooting with Jupyter Notebook on Windows, and found the same thing that you did, namely that installing ptyprocess via pip fixed the problem. |
@conda-forge/staged-recipes This Python recipe is ready for review |
ping @conda-forge/staged-recipes |
- ipython | ||
- notebook >=5.0.0 | ||
- pip | ||
- python >=3.6 |
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.
If this can't be noarch
you should add skip: True # [py<36]
to the build
section and specify remove the version constraint from Python:
- python >=3.6 | |
- python |
(same applies in the run
section)
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.
Having looked more closely it looks like this could be noarch: python
?
recipes/sos-notebook/meta.yaml
Outdated
sha256: 91fc849a9d8c0b5fb975ec82eb1f2b9be2603ff936b8241a928cc8d738a57e16 | ||
|
||
build: | ||
skip: True # [win] |
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 think this can be removed now as ptyprocess
is now noarch
:
skip: True # [win] |
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.
Indeed. Maybe they overheard our discussion. :-)
@jdblischak Could you try to remove this line and see if the tests pass?
This PR adds a recipe for the Python package sos-notebook.
The build scripts install the jupyter kernel. The
ipython
andnotebook
packages are included in the host dependencies because they are used by the script that installs the kernel (source).@BoPeng and @gaow, please confirm that you are willing to be co-maintainers of sos-notebook.