search

NSLS-II / nslsii

NSLS-II related devices
BSD 3-Clause "New" or "Revised" License
10 stars 21 forks source link

Allow 'nslsii' to load without IPython #108

Closed dmgav closed 3 years ago

dmgav commented 3 years ago

Insert conditions that verify if get_ipython() is None before trying to access IPython.get_ipython() attributes and methods. The changes should not affect how nslsii is loaded when IPython is available.

codecov-commenter commented 3 years ago

Codecov Report

Merging #108 into master will decrease coverage by 10.02%. The diff coverage is 47.05%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #108       +/-   ##
===========================================
- Coverage   66.66%   56.63%   -10.03%     
===========================================
  Files           1       13       +12     
  Lines           3      904      +901     
===========================================
+ Hits            2      512      +510     
- Misses          1      392      +391
Impacted Files Coverage Δ
nslsii/__init__.py 38.07% <30.76%> (ø)
nslsii/tests/conftest.py 100.00% <100.00%> (+33.33%) :arrow_up:
nslsii/tests/test_kafka_publisher.py 86.95% <100.00%> (ø)
nslsii/tests/temperature_controllers_test.py 68.75% <0.00%> (ø)
nslsii/common/ipynb/animation.py 18.75% <0.00%> (ø)
nslsii/temperature_controllers.py 63.26% <0.00%> (ø)
nslsii/common/ipynb/info.py 55.00% <0.00%> (ø)
nslsii/_version.py 44.80% <0.00%> (ø)
nslsii/tests/test_ipynb.py 100.00% <0.00%> (ø)
nslsii/common/ipynb/logutils.py 92.85% <0.00%> (ø)
... and 5 more

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 79ea5b8...dcac346. Read the comment docs.

tacaswell commented 3 years ago

It might make sense to call get_ipython() exactly once in the function or allow ip to be passed in, but this is a step in the right direction.

Travis failures are related to secrets in the .travis.yml not the code.

tacaswell commented 3 years ago

NVM, I was confused by the checks ui, we seem to failing for real with things related to kafka:

==================================== ERRORS ====================================
519________________________ ERROR collecting test session _________________________
520../../../virtualenv/python3.8.1/lib/python3.8/site-packages/_pytest/config/__init__.py:495: in _importconftest
521    return self._conftestpath2mod[key]
522E   KeyError: PosixPath('/home/travis/build/NSLS-II/nslsii/nslsii/tests/conftest.py')
523
524During handling of the above exception, another exception occurred:
525../../../virtualenv/python3.8.1/lib/python3.8/site-packages/py/_path/local.py:701: in pyimport
526    __import__(modname)
527../../../virtualenv/python3.8.1/lib/python3.8/site-packages/_pytest/assertion/rewrite.py:152: in exec_module
528    exec(co, module.__dict__)
529nslsii/tests/conftest.py:2: in <module>
530    from bluesky_kafka.tests.conftest import pytest_addoption, bootstrap_servers  # noqa
531E   ImportError: cannot import name 'bootstrap_servers' from 'bluesky_kafka.tests.conftest' (/home/travis/virtualenv/python3.8.1/lib/python3.8/site-packages/bluesky_kafka/tests/conftest.py)
532=============================== warnings summary ===============================
533/home/travis/virtualenv/python3.8.1/lib/python3.8/distutils/__init__.py:4
534  /home/travis/virtualenv/python3.8.1/lib/python3.8/distutils/__init__.py:4: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
535    import imp
536
537/home/travis/virtualenv/python3.8.1/lib/python3.8/site-packages/pims/image_reader.py:26
538  /home/travis/virtualenv/python3.8.1/lib/python3.8/site-packages/pims/image_reader.py:26: RuntimeWarning: PIMS image_reader.py could not find scikit-image. Falling back to matplotlib's imread(), which uses floats instead of integers. This may break your scripts. 
539  (To ignore this warning, include the line "warnings.simplefilter("ignore", RuntimeWarning)" in your script.)
540    warnings.warn(RuntimeWarning(ski_preferred))
541
542/home/travis/virtualenv/python3.8.1/lib/python3.8/site-packages/pims/cine.py:29
543  /home/travis/virtualenv/python3.8.1/lib/python3.8/site-packages/pims/cine.py:29: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated since Python 3.3, and in 3.9 it will stop working
544    from collections import Iterable
545
546-- Docs: https://docs.pytest.org/en/latest/warnings.html
547=========================== short test summary info ============================
548ERROR  - ImportError: cannot import name 'bootstrap_servers' from 'bluesky_ka...
549!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
550========================= 3 warnings, 1 error in 6.19s =========================
mrakitin commented 3 years ago

I think it has been failing on kafka tests before. @jklynch, do you think it's a new issue?

mrakitin commented 3 years ago

It might make sense to call get_ipython() exactly once in the function or allow ip to be passed in, but this is a step in the right direction.

I agree with Tom about ip and passing it through.

jklynch commented 3 years ago

This error looks like it comes from the optional command line argument --kafka-bootstrap-servers added to the pytest command line by bluesky_kafka/tests/conftest.py. It should not be necessary to specify this argument in .travis.yml.

mrakitin commented 3 years ago

Interesting... I don't see where it's used in https://github.com/NSLS-II/nslsii/blob/master/.travis.yml.

jklynch commented 3 years ago

@dmgav I failed to update the nslsii tests to match the latest release of bluesky_kafka. I pushed the necessary changes here.

jklynch commented 3 years ago

Now there is a new Kafka error related to connecting to the Kafka broker. I don't get this on my laptop - the tests pass there using the same docker-compose file to start Kafka.

----------------------------- Captured stderr call -----------------------------
%3|1601493970.455|FAIL|rdkafka#consumer-1| [thrd:127.0.0.1:9092/bootstrap]: 127.0.0.1:9092/bootstrap: Connect to ipv4#127.0.0.1:9092 failed: Connection refused (after 0ms in state CONNECT)
%3|1601493970.553|FAIL|rdkafka#consumer-1| [thrd:127.0.0.1:9092/bootstrap]: 127.0.0.1:9092/bootstrap: Connect to ipv4#127.0.0.1:9092 failed: Connection refused (after 0ms in state CONNECT, 1 identical error(s) suppressed)
%6|1601493974.867|FAIL|rdkafka#consumer-1| [thrd:127.0.0.1:9092/bootstrap]: 127.0.0.1:9092/bootstrap: Disconnected while requesting ApiVersion: might be caused by incorrect security.protocol configuration (connecting to a SSL listener?) or broker version is < 0.10 (see api.version.request) (after 1010ms in state APIVERSION_QUERY)
jklynch commented 3 years ago

The CI failure now is flake8.

dmgav commented 3 years ago

I fixed flake8. I will change the code to reduce the number of calls to get_ipython().

mrakitin commented 3 years ago

Thank you, @dmgav!

mrakitin commented 3 years ago

TravisCI builds pass. We don't currently worry about codecov failures.