Contribute examples to etsdemo#494
Conversation
…ove enable/savage examples into a common demo folder
|
|
||
| __extras_require__ = { | ||
| 'demo': ['chaco', 'mayavi', 'scipy', 'traitsui[demo]'], | ||
| 'demo': ['chaco', 'mayavi', 'scipy', 'traitsui[demo]', 'kiwisolver'], |
There was a problem hiding this comment.
This is a bit orthogonal to this PR, but so many examples depend on ConstraintsContainer that it felt appropriate to add. If one does pip install enable[demo] and pip install etsdemo[pyside2] and they launch the demo app, they would expect to have what they need to see all the demos.
I believe there are other dependencies as well (e.g. pyglet for pyglet_gl.py), but they are much less common. I may end up adding to them to this PR as well?
|
|
|
CI is failing on the enable added test, but I can't reproduce locally |
I recreated the environment and I can now reproduce the test failure, however if I run |
jwiggins
left a comment
jwiggins
left a comment
There was a problem hiding this comment.
Just some minor comments. I didn't look into the testing problems at all.
|
|
||
| if __name__ == '__main__': | ||
| demo = ScriptedComponentView() | ||
| demo.configure_traits() |
There was a problem hiding this comment.
I left original kiva_explorer.py sitting in examples as it is currently still best run outside of etsdemo. Further, in the etsdemo application the demo is still not shown here, all that is visible is the code at the moment.
Edit: this is now fixed, the full view is now shown in the demo but it is still a bit awkward looking as it is all condensed into the lower right panel
There was a problem hiding this comment.
I think the current status is about the best we are going to get with kiva_explorer in etsdemo for now. Eventually, we may want to rework things so that maybe when you click run the explorer application pops up separately or something like that.
I think I will delete the kiva_explorer.py that is still sitting in the original examples directory (actually I will delete examples entirely) as it can still be run the same way independent of etsdemo where it sits now.
On that note though, I am not a huge fan of where it sits now because it comes off as just another kiva example, but it is inherently a bit different, and would be nice if that distinction were clearer
| install_local = ( | ||
| "edm run -e {environment} -- " | ||
| "pip install --force-reinstall --no-dependencies -e ." | ||
| "pip install --force-reinstall --no-dependencies " + ROOT |
There was a problem hiding this comment.
I'm not exactly sure why, but I'm not seeing the test failure anymore
There was a problem hiding this comment.
Previously the test failure reflects the issue that I ran into 😅 : the examples files are moved inside demo subdirectory but the package_data has not been updated to say demo/enable/* etc.
Using pip install -e . "fixes" the test failure because it actually makes the package available from source: the package_data is not really relevant in that setup.
After 7cecefb, the package_data is fixed to reference the demo subdirectory, and that truly fixes the test.
This is in fact one instance where it is valuable to have a CI job where we don't install the source with editable mode. It tests the installed environment more truthfully.
There was a problem hiding this comment.
Ahhh I see, that makes sense now, thank you!
Codecov Report
@@ Coverage Diff @@
## master #494 +/- ##
==========================================
+ Coverage 30.63% 30.66% +0.03%
==========================================
Files 208 210 +2
Lines 17832 17840 +8
Branches 2454 2454
==========================================
+ Hits 5463 5471 +8
Misses 12036 12036
Partials 333 333
Continue to review full report at Codecov.
|
|
The and using EDIT: As a sanity check I copy pasted the code for |
|
https://github.com/enthought/enable/blob/7b56e5275c707e7ee5749ce4ba141240729f5a26/examples/enable/tools/apptools/undoable_move_tool.py related to enthought/traitsui#1428 EDIT: I changed |
|
https://github.com/enthought/enable/blob/7b56e5275c707e7ee5749ce4ba141240729f5a26/examples/kiva/pyglet_gl.py as well is not etsdemo friendly |
|
Note that https://github.com/enthought/enable/blob/7b56e5275c707e7ee5749ce4ba141240729f5a26/examples/kiva/qt4_simple.py fails with pyside2 with this error: But runs fine with pyqt5. However, Details |
kitchoi
left a comment
kitchoi
left a comment
There was a problem hiding this comment.
Some preliminary review comments. I understand this is still a work-in-progress. Happy to review this again.
aaronayres35
changed the title
kitchoi
left a comment
kitchoi
left a comment
There was a problem hiding this comment.
For the ones that don't work well, since we are exposing them anyway, what do you think about adding docstrings to warn about that instead of letting the user finding it out for themselves and getting disappointed?
e.g. https://github.com/enthought/traitsui/blob/98b6593d4e8eabb010982861ae977cc8a9d66f79/traitsui/examples/demo/Standard_Editors/TableEditor_demo.py#L5
It is also useful to add a comment referencing the issue you opened. https://github.com/enthought/traitsui/blob/98b6593d4e8eabb010982861ae977cc8a9d66f79/traitsui/examples/demo/Standard_Editors/TableEditor_demo.py#L17
Not only this will set the user's expectation upfront, later we can track down the issue and determine if the warning can be removed. e.g. I was able to remove one warning entirely when the sole issue is resolved: enthought/traitsui@1b7de78#diff-c79f163118f7b6def1f8f0f3fbff5736f8c1679f9d387c6dc4a411a8d437c887
It that sounds good to you, please feel free to do so in a separate PR as it is slightly broader scope.
SGTM! I might also mention on the ones that work stand-alone but not in etsdemo that changing |
Adding reference to |
kitchoi
dismissed
their stale review
Apologies. We agreed to resolve the issue noted in #494 (comment) here. Dismissing the review so we don't accidentally merge this. I will come back to review new changes.
| class Demo(DemoFrame): | ||
|
|
||
| def create_component(self): | ||
| def _create_component(self): |
There was a problem hiding this comment.
part of #500
All this required to fix was this leading underscore so I figured it was fine to add with this PR.
The issue name is misleading, as the other examples in the comments of the issue are still independently broken. they need to overwrite the _create_component method. I will update issue and try to fix those in a later PR
There was a problem hiding this comment.
This makes sense. Thank you!
| def _create_component(self): | ||
| canvas = Canvas(bgcolor="lightsteelblue", draw_axes=True) | ||
| from basic_move import Box | ||
| from enable.examples.demo.enable.basic_move import Box |
There was a problem hiding this comment.
@kitchoi I was googling namespace packages in python, and was trying to understand but still am a bit confused. It seems that doing this, without adding any __init__.py files works as we want it to, but I want to confirm I am not missing anything.
There was a problem hiding this comment.
Does from .basic_move import Box not work?
I don't think namespace packages are relevant in this situation.
There was a problem hiding this comment.
The context while running the example in the etsdemo GUI application is a little different.The Python path there is the Python path used for running the GUI application, which is not necessarily the same as the Python path used for running the script from the command line. The absolute import removes assumption on the Python path.
This is the block of code relevant while this file is run from the GUI application, the triple-underscore-main thing can be found here:
https://github.com/enthought/traitsui/blob/e913f98d10e96624245c603d856b33ded665d565/ets-demo/etsdemo/app.py#L462-L487
There was a problem hiding this comment.
It seems that doing this, without adding any init.py files works as we want it to
This was what I expected as well.
There was a problem hiding this comment.
@jwiggins comment is valid as in it shows the cognitive load required to maintain these examples.
Should enthought/traitsui#1455 be fixed, from .basic_move import Box should work (I think).
There was a problem hiding this comment.
Should enthought/traitsui#1455 be fixed,
from .basic_move import Boxshould work (I think).
That seems like the better solution here
There was a problem hiding this comment.
Sorry, I was wrong... I should never guess but try for myself... :(
from .basic_move import Box does not work, if I try to run the script from the command line (I am using master with the create_component -> _create_component fix:
$ pwd
/Users/kchoi/Work/ETS/enable/examples/enable
$ python canvas_demo.py
...
File "/Users/kchoi/Work/ETS/enable/enable/example_support.py", line 40, in _component_default
return self._create_component()
File "canvas_demo.py", line 10, in _create_component
from .basic_move import Box
ModuleNotFoundError: No module named '__main__.basic_move'; '__main__' is not a package
There was a problem hiding this comment.
The absolute import here does work on both the etsdemo application and from the command line.
from basic_move import Box does not work on etsdemo but will work if enthought/traitsui#1455 is fixed.
@jwiggins Is it okay for us to proceed with the absolute import here to decouple this import from the pending issue?
There was a problem hiding this comment.
From offline discussion: John says yes. :D
4 participants
fixes #436
This PR contributes the kiva and enable examples to etsdemo.
It is currently flagged as a WIP, as some examples are broken / have strange behaviors. However, some of these problems may best be solved in a separate PR.EDIT: I have opened issues for any broken demos, and have added comments to this PR for any demos that work when run independently, but not as we want them to when run through etsdemo.
To run the etsdemo app:
Additionally, some of the kiva examples simply save a file, rather than having a showcased demo. e,g.
dummy,pdf_arcs,star_path. Not sure what is best approach for these.I removed the WIP marker, as I think the lingering issues are likely best handled in separate PRs.