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

Move examples to package to be contributed to etsdemo #1275

Merged
merged 2 commits into from
Aug 27, 2020

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented Aug 13, 2020

This PR moves the "introduction" folder from examples/tutorials to the package so that they can be contributed to the etsdemo GUI application. It also adds the entry point needed for advertising the data.

If I install etsdemo and the traits from this branch with pip install .[examples], I can load the content in the etsdemo GUI application.
Screenshot 2020-08-13 at 14 39 05

Note that the examples in examples/tutorials/doc_examples and examples/tutorials/traits_4.0 are not moved:

The examples/tutorials/doc_examples folder provides mainly code that are used in the user manual. In other words, they are already published in a different medium and the documentation does a better job at explaining what is going on with these examples. If they are to be exposed in etsdemo, I think we need to move some of the descriptions in the documentation over to the example docstring so that they can appear as a description in the GUI. But maybe we don't need to duplicate them in the GUI application.

The examples/tutorials/traits_4.0 folder contains advanced usages as well as features we are planning to deprecate. So they require a bit of cleanup first. See #868.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
  • Update type annotation hints in traits-stubs

# dependencies for examples
"numpy",
"pillow",
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This installation option needs to be mentioned somewhere in the doc or readme.

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM with a few nitpicky comments.

@@ -323,15 +323,29 @@ def get_long_description():
"PySide2;python_version<'3.9'",
"traitsui;python_version<'3.9'",
],
"examples": [
Copy link
Contributor

Choose a reason for hiding this comment

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

"examples" or "demo"? traitsui and enable already has a demo section but pyface and chaco doesn't have any. It'll be useful settle on one for all ETS projects.

I'd go for "demo"

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "demo" in traitsui and enable are for the demo application, which is now the etsdemo.
Here the dependencies are for the content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah the one in enable includes both: Dependencies for the application, and dependencies for the content.
The existing one in traitsui is for the application only.

See enthought/traitsui#1140 where I try to separate the dependencies for the application and the dependencies for the content.

Here I also excluded etsdemo in the list. If one wants to run the examples, they don't necessarily have to run it via etsdemo.

"traits.tests": [
"test-data/historical-pickles/README",
"test-data/historical-pickles/*.pkl",
"test-data/historical-pickles/*.py",
],
},
entry_points={
"etsdemo_data": [
"introduction = traits._entry_point:introduction",
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, deciding on a common format across ETS projects might be useful here. we have 'demo = traitsui.extras._demo_info:info', in traitsui.

i mean a common format for the module name (i don't know if this is too much bikeshedding)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Traits can name these entry points however it wants. It can have many entry points if it likes.
I don't think a common format brings much values, in fact, it might be tempting to make assumptions about these names being consistently used.

},
ext_modules=[setuptools.Extension("traits.ctraits", ["traits/ctraits.c"])],
package_data={
"traits": [
"examples/introduction/*",
Copy link
Contributor

@rahulporuri rahulporuri Aug 13, 2020

Choose a reason for hiding this comment

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

i'm not sure why there is an traits/examples/introduction/__init__.py file and i don't think we need it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently it contains some text. That's how the demo application used to load the main description of a "folder".
They can be moved to an index.rst and then the __init__.py can be removed.
That should be a separate PR though.

@corranwebster
Copy link
Contributor

I am worried that that adding this to the main setup.py for Traits is going to cause problems. In particular, since Traits is used by the demo application, the entrypoint will be installed and always visible, but possibly not have the required dependent libraries. Similarly buildsystem issues arise (does Traits now need to require more things?)

My gut feeling is that the examples need to be their own package with their own setup.py that may not actually install any python packages, but does deliver the examples and the entry points, and which will have clear requirements.

@kitchoi
Copy link
Contributor Author

kitchoi commented Aug 13, 2020

I am worried that that adding this to the main setup.py for Traits is going to cause problems. In particular, since Traits is used by the demo application, the entrypoint will be installed and always visible, but possibly not have the required dependent libraries. Similarly buildsystem issues arise (does Traits now need to require more things?)

I think this can be solved by an application bundle that includes all the dependencies. Alternatively we can have a shell distribution that includes all the requirements without actually providing any Python package or example files.

The benefits of having Traits's examples hosted in Traits repository is that the examples can be versioned and updated at the same time as Traits changes, and it is much easier for navigating through examples in the same GitHub repository. If the examples are moved to another repository, it is going to be less discoverable. Ease of discovery is the reason why I retain a symbolic link in the examples folder in the source root tree in this repo.

@corranwebster
Copy link
Contributor

I wasn't suggesting a separate repository, just a separate distribution/egg/package - essentially a data only egg - but within the current repo. This would be similar to the current traits-stubs situation.

I agree that a new repo would be a bad idea.

A possible would be to ship the examples with traits, but have a stand-alone setup.py that contributed the extension point.

By bundling the Traits Examples in with Traits itself, it means that the etsdemo app can never be bundled for other demo code without having a copy of the Traits examples.

@kitchoi
Copy link
Contributor Author

kitchoi commented Aug 13, 2020

I see! I thought it would be nice to have some default content available in the demo application purely by virtue of the application's own dependencies. I personally think the missing dependencies on the content themselves (e.g. pillow here) are minor annoyance that can be detected from the run output and can be solved by an application bundle.

By bundling the Traits Examples in with Traits itself, it means that the etsdemo app can never be bundled for other demo code without having a copy of the Traits examples.

That is solved already. See the "Launch with specific data sources" section in https://pypi.org/project/etsdemo/

@corranwebster
Copy link
Contributor

By bundling the Traits Examples in with Traits itself, it means that the etsdemo app can never be bundled for other demo code without having a copy of the Traits examples.

That is solved already. See the "Launch with specific data sources" section in https://pypi.org/project/etsdemo/

But then the demo app ceases to be able to use the plugin capabilities.

Having default content available for the ets demo app out of the box could be solved by having optional dependencies for the app's installation.

I'd suggest perhaps leaving this open until @mdickinson is available for comment.

@mdickinson
Copy link
Member

I'd be inclined to leave the examples in the Traits package for now, both for simplicity and (as @kitchoi says) for discoverability. It's also consistent with what TraitsUI and Pyface already do.

By bundling the Traits Examples in with Traits itself, it means that the etsdemo app can never be bundled for other demo code without having a copy of the Traits examples.

I think that's fine. If at some later stage we have evidence that this is a real issue for a project, we can look at ways to mitigate it (and at that stage, we'll have more information about what the user needs actually are). A customer project that wants careful control may well want to circumvent the entry-point mechanism anyway.

@mdickinson
Copy link
Member

It's also consistent with what TraitsUI and Pyface already do.

Sorry. Just TraitsUI. Not Pyface.

@mdickinson mdickinson closed this Aug 27, 2020
@mdickinson mdickinson reopened this Aug 27, 2020
@mdickinson
Copy link
Member

Closing and re-opening to retrigger the CI (and pull in Sphinx 3.2.1 instead of the buggy Sphinx 3.2.0).

@kitchoi
Copy link
Contributor Author

kitchoi commented Aug 27, 2020

Thank you! Merging...

@kitchoi kitchoi merged commit 3413117 into master Aug 27, 2020
@kitchoi kitchoi deleted the move-examples-to-package branch August 27, 2020 16:09
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