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

Rework CI to be non-repetitive, simpler, test more things #2263

Merged
merged 6 commits into from
Nov 2, 2022

Conversation

llimeht
Copy link
Contributor

@llimeht llimeht commented Oct 25, 2022

This is a preview of some work to update the GitHub Actions, aiming to:

  • make them less repetitive
  • hopefully easier to maintain
  • updated to the latest versions of the helper-actions they call
  • able to be extended to do other testing more easily

This is only in a draft state, as this work is blocked by the following things:

(There's also another PR for pyinstaller that is queued behind #2259)

In the last steps of these workflows, I started to experiment with actual functional testing of the GUI and taking screenshots of the GUI in action. That's not working at the moment. I intend to back out that work and return to it later, in a separate PR.

@llimeht
Copy link
Contributor Author

llimeht commented Oct 26, 2022

I've split the functional GUI testing off. It's now ready for some review and discussion (but #2260, #2261, #2281 should go in before merging)

@llimeht llimeht marked this pull request as ready for review October 26, 2022 14:15
@llimeht llimeht changed the title Draft: Rework CI to be non-repetitive, simpler, test more things Rework CI to be non-repetitive, simpler, test more things Oct 26, 2022
.github/workflows/matrix.py Outdated Show resolved Hide resolved
installers/sasview.spec Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

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

Code looks fine. Artifacts from a recent build are here. The Mac installer runs sasview 5.0.5 as expected.

I don't know why the dmg is wrapped in a zip file. Presumably this reproduces the existing behaviour. Feel free to skip the zip or add a ticket to suggest doing so.

@pkienzle
Copy link
Contributor

Re: zip file, I suspect it is created by github to bundle the build artifacts from a workflow, so no need to change anything.

@llimeht llimeht force-pushed the tmp/cicombinematrix branch 2 times, most recently from 6547fda to d70a2d3 Compare October 28, 2022 14:51
@wpotrzebowski wpotrzebowski self-requested a review October 28, 2022 15:12
Copy link
Contributor

@wpotrzebowski wpotrzebowski left a comment

Choose a reason for hiding this comment

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

Good job!
The code looks good. One questions/suggestion I have is related to how we install packages and whether it can be done through pip install -r requirements.txt rather then listing them explicetly?

The mac installer works fine, however linux installer failed on ubuntu-20.04

Traceback (most recent call last):
  File "sasview.py", line 15, in <module>
  File "sas/qtgui/MainWindow/MainWindow.py", line 129, in run_sasview
  File "sas/qtgui/MainWindow/MainWindow.py", line 44, in __init__
  File "PyInstaller/loader/pyimod02_importers.py", line 499, in exec_module
  File "sas/qtgui/MainWindow/GuiManager.py", line 27, in <module>
  File "PyInstaller/loader/pyimod02_importers.py", line 499, in exec_module
  File "sas/qtgui/Utilities/GuiUtils.py", line 45, in <module>
  File "PyInstaller/loader/pyimod02_importers.py", line 499, in exec_module
  File "sasmodels/sasview_model.py", line 30, in <module>
    from . import weights
  File "PyInstaller/loader/pyimod02_importers.py", line 499, in exec_module
  File "sasmodels/weights.py", line 11, in <module>
    from scipy.special import gammaln  # type: ignore
  File "PyInstaller/loader/pyimod02_importers.py", line 499, in exec_module
  File "scipy/__init__.py", line 153, in <module>
  File "PyInstaller/loader/pyimod02_importers.py", line 499, in exec_module
  File "scipy/_lib/_ccallback.py", line 1, in <module>
  File "_ccallback_c.pyx", line 210, in init scipy._lib._ccallback_c
  File "PyInstaller/loader/pyimod02_importers.py", line 499, in exec_module
  File "ctypes/__init__.py", line 7, in <module>
ImportError: libffi.so.6: cannot open shared object file: No such file or directory
[29967] Failed to execute script 'sasview' due to unhandled exception!

@wpotrzebowski wpotrzebowski self-requested a review October 28, 2022 15:59
@wpotrzebowski
Copy link
Contributor

I accidently approved. I guess it may still require some work on linux side?

@butlerpd
Copy link
Member

Windows installer (#5) works well for me. Tested my standard 1D fitting which worked perfectly. The only issue was that I could not run the python shell which gave the following error:

FileNotFoundError: [WinError 3] The system cannot find the path specified: 'c:\SasView-5.0.6a\debugpy\_vendored'

I suspect that is not related to this PR but more to something done earlier (and now reversed)?

@llimeht
Copy link
Contributor Author

llimeht commented Oct 29, 2022

The mac installer works fine, however linux installer failed on ubuntu-20.04

Yes, that's a new failure that has appeared during code camp. I'm not sure if that's due to lack of CI or some other external change.

@llimeht
Copy link
Contributor Author

llimeht commented Oct 29, 2022

@wpotrzebowski

Good job! The code looks good. One questions/suggestion I have is related to how we install packages and whether it can be done through pip install -r requirements.txt rather then listing them explicetly?

Perhaps. That's a can of worms in itself and can be a separate PR. The objective of this is to do what we already doing but in a simpler fashion. I'm trying avoid scope creep and this exploding into much larger and longer discussions.

@llimeht
Copy link
Contributor Author

llimeht commented Oct 29, 2022

@wpotrzebowski: linux installer updated; looks like some library changes that pyinstaller wasn't quite catching up with, so a little tweak with a sover symlink farm there solves it.

@butlerpd: added in the debugpy._vendored hidden import again; that might be enough to the console working, but I have a recollection that it wasn't quite enough in the past.

(And now I've got to include #2326 in commits for this PR otherwise the test suite fails)

@wpotrzebowski
Copy link
Contributor

@llimeht thanks for fixes!

I will test the installer.

I think we don't need debugpy installed explicitily via pip (it is pulled by qt_consoloe). It is probably enough to add debugpy._vendored to the spec file but I am fine with the current version too.

@butlerpd
Copy link
Member

Not sure I understand (OK .. I am sure I don't understand:-) but testing the installer on windows I still see the same error and indeed the path to: C:\SasVIew\debugpy does not exist (i.e. there is no folder debugpy in the SasVIew folder. Afraid I cannot be of much more help. Maybe @wpotrzebowski has a thought?

@wpotrzebowski
Copy link
Contributor

The linux install installer works now besides python shell editor (debugpy._vendored error as reported by @butlerpd ). Not sure if it helps in this case but should we tell pyinstaller to explicetly add it to installer: datas.append((os.path.join(PYTHON_PACKAGES, 'debugpy'), 'debugpy')) as we do it currently for Mac?

@wpotrzebowski
Copy link
Contributor

MacOSX installer seems to work fine (in sasme cases even better) with latest PyQt version. No need to bind it to 5.13 anymore.

@llimeht
Copy link
Contributor Author

llimeht commented Oct 30, 2022

Updated once more (but perhaps this is a topic for bug fixes for a separate PR)

@wpotrzebowski
Copy link
Contributor

wpotrzebowski commented Oct 30, 2022

I've tested OSX and linux installers and they seem to work fine up to the point when one starts python shell editor. The editor itself works fine however the error occurs:

5:34:41 - ERROR: Traceback (most recent call last):
  File "sas/sascalc/data_util/calcthread.py", line 272, in _run
  File "sas/qtgui/Perspectives/Fitting/ModelThread.py", line 179, in compute
  File "sasmodels/sasview_model.py", line 713, in calculate_Iq
    return self._calculate_Iq(qx, qy)
  File "sasmodels/sasview_model.py", line 721, in _calculate_Iq
    self.__class__._model = core.build_model(self._model_info)
  File "sasmodels/core.py", line 335, in build_model
    source = generate.make_source(model_info)
  File "sasmodels/generate.py", line 1025, in make_source
    lineno = getframeinfo(currentframe()).lineno + 2
  File "inspect.py", line 1473, in getframeinfo
  File "PyInstaller/hooks/rthooks/pyi_rth_inspect.py", line 25, in _pyi_getsourcefile
AttributeError: module '__main__' has no attribute '__file__'
15:34:41 - ERROR: Traceback (most recent call last):
  File "sas/sascalc/data_util/calcthread.py", line 272, in _run
  File "sas/qtgui/Perspectives/Fitting/ModelThread.py", line 179, in compute
  File "sasmodels/sasview_model.py", line 713, in calculate_Iq
    return self._calculate_Iq(qx, qy)
  File "sasmodels/sasview_model.py", line 721, in _calculate_Iq
    self.__class__._model = core.build_model(self._model_info)
  File "sasmodels/core.py", line 335, in build_model
    source = generate.make_source(model_info)
  File "sasmodels/generate.py", line 1025, in make_source
    lineno = getframeinfo(currentframe()).lineno + 2
  File "inspect.py", line 1473, in getframeinfo
  File "PyInstaller/hooks/rthooks/pyi_rth_inspect.py", line 25, in _pyi_getsourcefile
AttributeError: module '__main__' has no attribute '__file__'

Not sure how difficult to fix it is, however I would vote for merging it and addressing this issue in a separate PR.

@wpotrzebowski
Copy link
Contributor

I would vote for merging it, however I would wait until end of the code camp (tomorrow).

@pkienzle
Copy link
Contributor

pkienzle commented Nov 1, 2022

I suspect the error is a result of the python app builder putting modules in a zip file. IIRC the import hook which loads the module from the zip does not define __file__. That doesn't explain why the problem is showing up now.

The code which is failing is not critical. It is telling the model compiler that the code is coming from the current line in generate.py so that it'll be easier to interpret compiler errors. As a short term fix we can wrap this in an exception handler and use a hard-coded line number as the default. It doesn't matter if the offset drifts a little as the code is modified; just knowing that the code is coming from generate.py should be enough to debug any problems.

Seems to have come back via a bad merge.
- reduce duplication
- easier maintenance of test matrix
- test more things
Should not be needed but without it, libffi.so.6 is not found.
@llimeht
Copy link
Contributor Author

llimeht commented Nov 1, 2022

Updated once more for merge conflicts.

@wpotrzebowski
Copy link
Contributor

Tested latest OSX installer. The convoluted issue with shell editor still persists but I am merging it as it is.

@wpotrzebowski wpotrzebowski merged commit 70434fb into SasView:main Nov 2, 2022
@llimeht llimeht deleted the tmp/cicombinematrix branch November 3, 2022 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants