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

Multi module support #272

Merged
merged 8 commits into from
Jul 26, 2021
Merged

Multi module support #272

merged 8 commits into from
Jul 26, 2021

Conversation

mbway
Copy link
Contributor

@mbway mbway commented Jun 19, 2021

I have added support for a multi-module workflow to CQ-Editor by:

  • removing modules that are imported by the main module (otherwise the parts from the non-main modules will never update as their modules are cached by the interpreter since the scripts and the editor run in the same interpreter)
  • searching for and watching the paths of any modules imported by the main module, and triggering a re-render whenever any of the watched files are changed

I'm not sure if this is a common workflow (likely it won't be since the tooling doesn't support it well) but if I want to create a complex part with multiple components, I might want to have separate modules where I design each part in a stand-alone module which can be run to create just that part, and a main script which assembles everything together.

old_behaviour.mp4
multi_module.mp4

@codecov
Copy link

codecov bot commented Jun 19, 2021

Codecov Report

Merging #272 (cf68d38) into master (fbf8c8c) will increase coverage by 0.10%.
The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #272      +/-   ##
==========================================
+ Coverage   88.23%   88.33%   +0.10%     
==========================================
  Files          19       19              
  Lines        1436     1466      +30     
  Branches      167      175       +8     
==========================================
+ Hits         1267     1295      +28     
  Misses        139      139              
- Partials       30       32       +2     
Impacted Files Coverage Δ
cq_editor/widgets/debugger.py 82.15% <95.83%> (+0.12%) ⬆️
cq_editor/widgets/editor.py 95.86% <96.66%> (-0.04%) ⬇️

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 fbf8c8c...cf68d38. Read the comment docs.

@adam-urbanczyk
Copy link
Member

Thanks for the PR, this goes quite deep so I'm not sure I'm comfortable with merging it at this point. How reliable is the approach with using del on sys.modules?

@mbway
Copy link
Contributor Author

mbway commented Jun 19, 2021

I agree it isn't ideal. I couldn't find any examples of applications where the interpreter is shared between the application and the user scripts so doing this kind of clearing is very uncommon I imagine.

The documentation does say This can be manipulated to force reloading of modules and other tricks so it is intended functionality even if it is a bit hacky.

But equally, by not clearing the state after running the user script can leave the editor itself in a changed state so not clearing has it's own problems.
Another approach could potentially be using importlib.reload_module with the knowledge that the module will have been loaded the first time around. This isn't necessarily sound though if the user script changes it's imports and has the same issue of lingering 'cached' modules after the script has run.

During my research I came across this which clearly says "don't delete from sys.modules" but I don't think that advice is applicable here. Not touching sys.modules is absolutely the correct approach in regular programs, but what CQ-editor is doing here is essentially running an entirely different program using the current interpreter, so worrying about dangling references does not apply.

The ideal solution would be to sandbox the user script in some way or spawn an entirely separate python interpreter to run the user script in. I don't know how to go about this though. If the cadquery data structures were picklable then the data could be sent back to the editor by pickling and sending over a pipe or socket or something but the OCCT wrappers would have to support pickling which they currently do not.

@adam-urbanczyk
Copy link
Member

adam-urbanczyk commented Jun 19, 2021

With CadQuery/cadquery#735 almost merged it will be possible to (de)serialize OCCT models. This would open the possibility of executing the code in a different ipython kernel and rather efficiently getting the results into CQ-editor. I have that one on a virtual TODO list but won't happen anytime soon.

It seems that spyder is also using the del approach: https:/spyder-ide/spyder/blob/01d9195b1a33d02f42faf3c8e0047412966a24ce/external-deps/spyder-kernels/spyder_kernels/customize/umr.py#L135 so I guess it is not too crazy.

Do you think that reloading on a dep is really desired? Intuitively I'd be against doing that.

@mbway
Copy link
Contributor Author

mbway commented Jun 19, 2021

well clearly if that module has changed, it should be reloaded? I as a user expect that when I click 'run' on my script in an IDE or editor, that I am running my script fresh, running the code as it currently exists on the filesystem, not using the code as it was the first time I clicked run in my IDE. That confusion is what led me to write this feature.

You might want to not reload system imports. I would argue this is a bit of a premature optimisation but system and non-system modules could be distinguished if you prefer?

@adam-urbanczyk
Copy link
Member

Sorry, I meant re-rendering on a dep change.

@mbway
Copy link
Contributor Author

mbway commented Jun 19, 2021

That's a good point. I think it's less surprising for users that when 'autoreload' is enabled, any change they make to their model is automatically updated in the editor (regardless of where that change is made), but for large complex models it might make sense to have more control about when the re-rendering is triggered. Perhaps there should be a user preference for this?

@adam-urbanczyk
Copy link
Member

Yes, I'd indeed propose to add another option to enable this behavior (and I'd keep it off by default ). Regarding the automatic reloading - it might be also good to keep it as an option (on by default).

@mbway
Copy link
Contributor Author

mbway commented Jun 19, 2021

How is this?

There is a slight issue where setting 'Cache imported modules' to True will taint that session (already cached modules cannot be un-cached) because of the way the module unloading currently works. I could alter this to do something like tag the user imported modules regardless of the preference but only unload them depending on the preference. Or it could just be understood that the editor needs to be closed and re-opened after changing this setting.

@adam-urbanczyk
Copy link
Member

You could implement the updatePreferences method to handle this.

@mbway
Copy link
Contributor Author

mbway commented Jun 19, 2021

I mean, without always recording what modules are loaded by the user script, I wouldn't know what modules to unload. I could implement that but it would add more complexity

@adam-urbanczyk
Copy link
Member

Got it, that won't be needed. I was thinking about unwatching the watched dep module files, but I guess it's not worth the complication.

@mbway
Copy link
Contributor Author

mbway commented Jun 19, 2021

the watch / don't watch imported modules preference can be set and unset properly at runtime, it's just the cache / don't cache imported modules that can't (going False -> True works fine, going True -> False does not)

@adam-urbanczyk
Copy link
Member

FYI: I'll try to add some more tests and will definitely merge this.

@adam-urbanczyk
Copy link
Member

Thanks @mbway !

@adam-urbanczyk adam-urbanczyk merged commit e16d5ee into CadQuery:master Jul 26, 2021
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.

2 participants