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

Update API to support echo>=0.6 and vispy=0.11 #373

Merged
merged 5 commits into from
Oct 21, 2022

Conversation

dhomeier
Copy link
Contributor

Description

This adds fixes for some upstream API changes which should fix a large part of the currently failing tests:

  1. list has been removed from echo
  2. Move module globals to class attributes vispy/vispy#2227 (comment) ; pulling in 9034cd3 from Switch CI to GitHub actions #371 for this.
  3. vispy 0.10 -> 0.11 has replaced the CatRom filter with CatRom2D, breaking most of test_vispy_toolbar and also reported in 3d scatter fails to plot with PyQt5 and GL errors glue#2311
    Making the latest vispy version required for the latter; for echo there currently is no explicit dependency at all (probably pulled in from glue-core), but it's probably unlikely to run into a version < 0.2 now.
    _
    This leaves two points of test failure:
  4. test_record reporting errors on writing to a closed file, not sure yet what to make of this:
>       assert err.strip() == ""
E       assert '--- Logging ...Arguments: ()' == ''
E         + --- Logging error ---
E         + Traceback (most recent call last):
E         +   File "/Users/derek/opt/mambaforge/envs/py39/lib/python3.9/logging/__init__.py", line 1086, in emit
E         +     stream.write(msg + self.terminator)
E         +   File "/Users/derek/opt/mambaforge/envs/py39/lib/python3.9/site-packages/glue/app/qt/application.py", line 157, in write
E         +     self._stderr_original.write(message)
E         + ValueError: I/O operation on closed file....
  1. Isosurface viewer failure, leading down to a whole bunch of incompatibilities between vispy's VolumeVisual and what is currently implemented in MultiIsoVisual; putting this off to a separate PR.

@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #373 (1502b14) into master (9034cd3) will decrease coverage by 0.38%.
The diff coverage is 42.50%.

@@            Coverage Diff             @@
##           master     #373      +/-   ##
==========================================
- Coverage   80.48%   80.10%   -0.39%     
==========================================
  Files          50       50              
  Lines        3885     4016     +131     
==========================================
+ Hits         3127     3217      +90     
- Misses        758      799      +41     
Impacted Files Coverage Δ
glue_vispy_viewers/compat/text.py 88.41% <ø> (+0.69%) ⬆️
...viewers/isosurface/tests/test_isosurface_viewer.py 37.31% <16.00%> (+6.16%) ⬆️
...e_vispy_viewers/common/tests/test_vispy_toolbar.py 25.26% <50.00%> (-57.50%) ⬇️
..._vispy_viewers/common/tests/test_3d_axis_visual.py 100.00% <100.00%> (ø)
...ue_vispy_viewers/common/tests/test_vispy_viewer.py 97.01% <100.00%> (+0.40%) ⬆️
...ue_vispy_viewers/common/tests/test_vispy_widget.py 100.00% <100.00%> (ø)
...vispy_viewers/scatter/tests/test_scatter_viewer.py 100.00% <100.00%> (ø)
glue_vispy_viewers/conftest.py 78.12% <0.00%> (-15.63%) ⬇️
glue_vispy_viewers/common/tools.py 59.49% <0.00%> (-11.40%) ⬇️
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dhomeier
Copy link
Contributor Author

dhomeier commented Jul 27, 2022

@astrofrog I got the local tests under macOS/Python 3.9 down to 4 failed, 33 passed with this, so would like to get #371 merged and rebase to check if all the teardown errors with linux/macos here are due to the Azure setup.

@dhomeier dhomeier requested a review from astrofrog July 27, 2022 22:34
@dhomeier dhomeier force-pushed the echo-vispy-updates branch 9 times, most recently from a951cb5 to 78e0cc7 Compare July 28, 2022 01:15
@dhomeier
Copy link
Contributor Author

Not pretty, but disabling the broken tests from (•4, •5) got rid of the teardown errors.
Still should be checked against the Github CI as well.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhomeier - now that #371 is merged, could you rebase this? Please tag me for review again once this is done

@dhomeier dhomeier force-pushed the echo-vispy-updates branch 9 times, most recently from fd235b6 to 1edb0ec Compare August 23, 2022 16:58
@dhomeier dhomeier force-pushed the echo-vispy-updates branch 4 times, most recently from bf78c73 to 6916a6a Compare August 23, 2022 17:38
@dhomeier
Copy link
Contributor Author

@astrofrog some more issues came up after rebasing:

  1. Windows has no PyQt5 5.14 for py39 and a broken one for py310 (see last test failure here). Could move everything to 5.15 with all the xcb* deps, or just conditionally for sys_platform=='win32'.
  2. Windows runs are failing on (to me) obscure runtime errors

.....tox\py38-test-dev\lib\site-packages\glue_vispy_viewers\scatter\tests\test_scatter_viewer.py s [ 45%]
sWindows fatal exception: access violation
Thread 0x000001c8 (most recent call first):
File "D:\a\glue-Windows fatal exception: vaccess violationi
spy-viewers\glue-vispy-viewers.tox\py38-test-dev\lib\site-pacERROR: InvocationError for command 'D:\a\glue-vispy-viewers\glue-vispy-viewers.tox\py38-test-dev\Scripts\pytest.EXE' --pyargs glue_vispy_viewers --cov glue_vispy_viewers '--cov-report=xml:D:\a\glue-vispy-viewers\glue-vispy-viewers\coverage.xml' (exited with code 3221225477)

They appeared actually already in #371, just missed them among the other failures. Also, the last Azure runs for this PR showed a bunch of exceptions/warnings like

RuntimeError: Using glBindFramebuffer with no OpenGL context.
ValueError: I/O operation on closed file.

however still returned as "passed". I have skipped those tests on win32, but am now well into test_scatter_viewer, which did pass without any warnings in Azure, so there still seems to be something missing from the GH Actions setup on Windows. With no further backtrace, no idea what to do here except skipping possibly the entire rest of the tests for win32.

@dhomeier
Copy link
Contributor Author

@astrofrog I was not able to restart the windows runs here to check if those problems are still present, but I think this should perhaps be merged to ensure at least vispy 0.11 support.

@dhomeier dhomeier merged commit b7c4915 into glue-viz:master Oct 21, 2022
@dhomeier dhomeier changed the title Some updates to run with current echo and vispy Update API to support echo>=0.6 and vispy=0.11 Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants