Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#1142 closed defect (fixed)

Plugin framework is broken

Reported by: butler Owned by: pkienzle
Priority: blocker Milestone: SasView 4.2.1
Component: SasView Keywords:
Cc: Work Package: SasView Bug Fixing

Description (last modified by butler)

several people have mentioned problems with plugins recently. Thorough testing, particularly by @tcbennun and @gonzalezm, has revealed that the problem seems to lie in the loading of the plugin and the math package (quoting from @gonzalezm):

  1. I create a simple model without math functions (A+B*x): OK
  1. Using a math function as in the tiptool (A+B*cos(2*pi*x)): The editor gives a success message and the python file is correctly generated and saved in plugin_models. But when trying to load it in SasView, an error appears:
2018-07-20 12:11:54 : Traceback (most recent call last):
File "sas\sascalc\data_util\calcthread.pyc", line 274, in _run
TypeError: unsupported operand type(s) for *: 'NoneType' and 'float'
  1. I edited the generated file to check it and everything looks correct. Then it is enough to modify it (e.g. add a space) or save it with a different name, and then the model can be loaded and it will work.
  1. As the model is correct, closing and relaunching SasView will also make the model available and it works. This could be a roundabout, i.e. first create the model, then close SasView and then restart. But it is clearly not satisfactory.

Investigating further it seems the root cause may be the loading for testing of new plugins (quoting now from @tcbennun):

It seems that when you create a new plugin model, the new Python file is loaded as a module from source twice when it is checked, in, check_model(). The first time, it is loaded with, load_custom_model() (just to check it can be run), and the second time, with, load_kernel_module() (this happens during the unit tests).

If load_custom_model() had been used twice there would be no problem, because that function caches previously-loaded modules and prevents them from being reloaded. But load_kernel_module() is separate, so the module is loaded again.

The GUI generally only uses load_custom_model(), as far as I can tell, so this bug might not appear elsewhere at all.

Theoretically this problem shouldn’t exist (Python docs claim that reloading a module from source re-initializes the module safely), but we’re seeing that the module’s namespace is not populated correctly for whatever reason the second time round, so it’s missing those maths functions which are imported at the top of the file.

Further testing by myself shows that actually any plugin, if tested (either by the new editor module because it is just created or by running run → Check model in the advanced model editor) once, may pass the test (it will not the second time) but will now throw errors when used. This includes downloading a model from the marketplace. Just running it as is works, as soon as it is loaded into the editor and checked (regardless of whether changes have been made) it no longer works. Exiting SasView and restarting clears the problem.

This issue is probably identical to the one that prevented #885 from being implemented (in fact we rolled back polydispersity completely). Once this gets fixed we should retest

Attachments (2) (1.1 KB) - added by butler 3 years ago.
Maria's original version (1.5 KB) - added by butler 3 years ago.
New version of Maria's model

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by butler

  • Owner set to pkienzle
  • Status changed from new to assigned

Further sleuthing with the help of Paul Kienzle using the built in python console confirms Torin's findings: there are two separate loaders involved which for some reason both want to keep a separate copy of the model. It appears that when the sasmodels unit tests load the model the second time the math globals are overwritten so that rather than functions they become "None" objects. Unfortunately, as I understand it, SasView uses the first version (where all the math functions are now "NONE" type objects) leading to the errors being thrown.

Paul Kienzle points out this is probably an old problem going back to the Grenoble code camp where it was found that numpy needed to be imported within the I(q) function itself to work? He also thinks he can force sasmodels to use the version loaded by SasView so that it is not loaded twice which should fix the issue.

The longer term question however, that maybe should become a shorter term one, is that sasmodels.sasview was meant as a temporary shim so that we could release SasView updates while in the process of converting to the new sasmodels package properly. With version 4.2.0, that conversion is essentially complete and thus the time has come that for that shim to disappear and for SasView to call sasmodels directly rather than through .sasview. But that is a fair amount of work and 4.2.0 will clearly still be using the shim. Version 5 however should definitely be working to sasmodels?

comment:2 Changed 4 years ago by butler

  • Description modified (diff)

comment:3 Changed 3 years ago by Paul Kienzle <pkienzle@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 2a6058cb3b646db9e7b57a6ce2ff246d32fa5518/sasview:

fix broken plugin model tests. Fixes #1142

comment:4 Changed 3 years ago by Paul Kienzle <pkienzle@…>

In fa374b0f1a207d4b01b7cb3dc0bfc3cef6bc64fa/sasview:

Save before pushing… Fixes #1142

comment:5 Changed 3 years ago by butler

  • Resolution fixed deleted
  • Status changed from closed to reopened

The fix added in SasView PR 161 seems to fix part of the problem? maybe? but not really? Attached are two files, one from Maria Valldeperas in Lund clearly using a slightly older version of the plugin editor, but trivially so. The other created by copying and pasting all the bits from Maria's version using the new model editor using the PR build (so the 4.2.0-beta version of the editor). Maria's throws the usual error of the math functions being of type "NoteType" while the new one (which looks very similar) does not. Note that for Maria's model there is no need to load it in the editor and run the test. Just dropping it in the plugin folder and launching SasView then try choosing that plugin from the fitpanel. Moreover, by removing the math functions from Maria's original file (cos and exp) and leaving simple arithmetic, the model loads and runs fine - further suggesting it is the same problem with the math functions.

a couple of questions:

  • Do others see the same thing?
  • If so, does this suggest the problem still lurks and depending on the user could pop up again even with the "new" editor?
  • If not, should we abandon all hope of backward compatibility? I find this a bit troubling given the almost identical nature of these files.
Last edited 3 years ago by butler (previous) (diff)

Changed 3 years ago by butler

Maria's original version

Changed 3 years ago by butler

New version of Maria's model

comment:6 Changed 3 years ago by Paul Kienzle <pkienzle@…>

In e2663b7a7ed44d36d60ec897fa92d1309bcf6d8b/sasview:

remove cached model before attempting reload. Refs #1142

comment:7 Changed 3 years ago by pkienzle has a mixture of line endings:


Turns out that this is a red herring, and the problem was elsewhere, but we may want to figure out whether it is our editor which is doing this.

comment:8 Changed 3 years ago by GitHub <noreply@…>

In 3bfcd9ea3b637986d859554ea178d94819486a2f/sasview:

Merge pull request #161 from SasView?/ticket-1142-plugin-reload

Ticket 1142 plugin reload

addresses #1142 hopefully for the last time - actually will leave the ticket open as there is an improved fix that is waiting in a sasmodels PR to be merged after the release - move ticket to 4.3 now

comment:9 Changed 3 years ago by butler

  • Resolution set to fixed
  • Status changed from reopened to closed

Actually, in hindsight this will be too confusing so have opened another ticket for the refactoring and will now close this one.

comment:10 Changed 3 years ago by GitHub <noreply@…>

In c6084f192171c34cf1111bfae238f49f5ebf15f5/sasmodels:

Merge pull request #75 from SasView?/ticket-1142-plugin-reload

refactor tests so that gui doesn't reload module each time. Refs #1142.

comment:11 Changed 3 years ago by butler

  • Milestone changed from SasView 4.2.0 to SasView 4.2.1

Some fixes for 4.2.1 added after closing moving to 4.2.1

Note: See TracTickets for help on using tickets.