enthought / traits-futures Goto Github PK

The tests require several additional packages: mock, PySide (or PyQt), and traitsui. We should specify those in extras_require.

We need to provide an API to do controlled shutdown and cleanup.

Note: this particularly affects the pi iterations example introduced in #38.

Before we can make the repo public, we need to get rid of the current CI system and replace it with something simpler, possibly based on the current etstool.py used in Traits / Envisage.

We should consider using the flake8 import plugin to enforce import order.

The classes have evolved so that they no longer give a good description of what they're for. Rename!

It looks as though #153 introduced intermittent test failures: run_until sometimes exits before the awaited condition is true. I suspect that this is due to the singleshot timer that was supposed to check the condition at the beginning of the event loop - my guess is that as a result, we have events being left on the GUI event queue at the end of some tests, and those events then interact with one of the following tests.

This was observed with Python 3.8.3 and PySide2:

(traits-futures) mirzakhani:traits-futures mdickinson$ python --version
Python 3.8.3
(traits-futures) mirzakhani:traits-futures mdickinson$ pip list
Package        Version
-------------- ----------
pip            20.1.1
pyface         7.0.0
PySide2        5.15.0
repeat         0.1.0
setuptools     49.1.2
shiboken2      5.15.0
six            1.15.0
traits         6.1.0
traits-futures 0.2.0.dev0
wheel          0.34.2

Here's an example of a test run failure:

(traits-futures) mirzakhani:temp mdickinson$ python -m unittest discover traits_futures
...........................F..E....................................................................
======================================================================
ERROR: test_failing_iteration (traits_futures.tests.test_background_iteration.TestBackgroundIteration)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdickinson/.venvs/traits-futures/lib/python3.8/site-packages/traits_futures/tests/background_iteration_tests.py", line 179, in test_failing_iteration
    self.assertException(future, ZeroDivisionError)
  File "/Users/mdickinson/.venvs/traits-futures/lib/python3.8/site-packages/traits_futures/tests/background_iteration_tests.py", line 439, in assertException
    self.assertEqual(future.exception[0], str(exc_type))
  File "/Users/mdickinson/.venvs/traits-futures/lib/python3.8/site-packages/traits_futures/background_iteration.py", line 194, in exception
    raise AttributeError("No exception has been raised for this call.")
AttributeError: No exception has been raised for this call.

======================================================================
FAIL: test_cancel_during_iteration (traits_futures.tests.test_background_iteration.TestBackgroundIteration)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdickinson/.venvs/traits-futures/lib/python3.8/site-packages/traits_futures/tests/background_iteration_tests.py", line 226, in test_cancel_during_iteration
    self.assertEqual(
AssertionError: ['waiting', 'executing', 'cancelling'] != ['waiting', 'executing', 'cancelling', 'cancelled']

----------------------------------------------------------------------
Ran 99 tests in 1.023s

FAILED (failures=1, errors=1)

The test run fails on my machine approximately one run in three.

If job submission raises an exception (as it's expected to do for example if a progress keyword is passed to submit_progress), then a new message receiver is registered but never cleaned up.

Related: #78.

>>> from traits_futures.api import TraitsExecutor
>>> executor = TraitsExecutor()
>>> len(executor._message_router._receivers)
0
>>> executor.submit_progress(int, progress=3)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mdickinson/Desktop/traits-futures/traits_futures/traits_executor.py", line 138, in submit_progress
    return self.submit(task)
  File "/Users/mdickinson/Desktop/traits-futures/traits_futures/traits_executor.py", line 161, in submit
    message_receiver=receiver,
  File "/Users/mdickinson/Desktop/traits-futures/traits_futures/background_progress.py", line 320, in future_and_callable
    raise TypeError("progress may not be passed as a named argument")
TypeError: progress may not be passed as a named argument
>>> len(executor._message_router._receivers)
1

Travis (and possibly appveyor too) allow the creation of cron jobs. Some of the ETS packages now have cron jobs setup to test against dependencies which are installed from source - instead of testing against the released versions.

See examples :

• enthought/envisage#162
• enthought/traitsui#914
• enthought/pyface#549

If submit_iteration is used to run a generator in the background, we need to ensure that that generator is given the opportunity to perform proper cleanup. (E.g., imagine the case where the generator opens a file for reading line-by-line, and then the job gets cancelled.)

This is easy to do: we just make sure to call the generator's close method explicitly. The catch is that submit_iteration supports general iterables, and general iterables don't have a close method (or, even worse, perhaps a user-implemented iterable does have a close method, and that close method doesn't do anything sane).

Also, in current CPython, it's enough to ensure that there are no lingering references to the generator, since its close method automatically gets called at garbage collection time. We should definitely make sure that there are no lingering references, but we also shouldn't rely on this implicit cleanup.

I'll probably go with checking whether the callable is an instance of types.GeneratorType, and calling its close method if so. It's a bit of a violation of EIBTI, though.

Add a background iteration task, based on #21. The code in #21 is outdated since much refactoring has happened, but should be good in principle.

Suggest an example based on computing successive approximations to π via Monte Carlo, with an automatically updating plot showing the last few approximations.

The CallFuture and ProgressFuture have a result attribute that provides the result of the background call. It's a Python property (not a trait), that raises AttributeError if you try to access it before the call has finished.

The IterationFuture has a result trait (an Event), that gets fired as each iteration result comes in.

To avoid confusion (and to allow for the possibility of an iteration that also returns a result later on), we should rename one or other of these.

IterationFuture.iterate?

@corranwebster @rkern @jvkersch Any opinions? Naming things is hard 😒

On Python 3, we shouldn't need to depend on the PyPi mock package.

Candidates for backport for the 0.1.1 release:

• #116 (long description field in setup.py)
• #114 (add copyright headers)
• #102 (fix an incorrect listener specification in a test) (this one doesn't apply cleanly)
• #99 (remove unnecessary bundle generation machinery)

Release steps:

• Make release/0.1.x maintenance branch, starting from the release/0.1.0 tag
• Backport the above fixes (#118)
• Update changelog (#119)
• Bump version number (#120)
• Test
• Tag release
• Upload PyPI eggs
• Test the PyPI eggs
• Announce the release!
• Bump version number again for continued development Not necessary, since we're not changing master
• Merge release branch back into master. Not this time; too messy for this release

There's some machinery in marshal_exception to turn a background exception into something that can be transmitted across thread/process boundaries. We need to check that this is handing Unicode (e.g., in exception messages, in code formatted into the traceback, etc.) correctly on both Python 2 and Python 3.

The last portion of the README needs an update.

In the TraitsExecutor, it might be worth keeping track of the actual concurrent.futures Future objects (perhaps as an attribute in the associated Future), and checking their exception state on completion (via add_done_callback). It would complicate the code, but the benefits are:

1. Better cleanup/shutdown: we can be sure that the worker tasks are no longer running after the TraitsExecutor stops. This is especially useful in the case of a shared executor. (For a private executor, we can shut down the executor to be sure that no futures are still running).
2. Easier debugging during traits-futures development: currently, if there's a programming error in the CallFuture code (for instance), we get silent failures or hangs, because we never check the concurrent futures objects for failure.

Suggested by @jvkersch, and an issue on real project:

Introduce a new type of background job / future pair for background processes fired off with a subprocess call. The future would report:

• the state of the background process (running / completed / errored)
• the process return code for a completed process
• text sent to stdout / stderr by the background process (could include progress messages, ...)

We're using "completed" both as a property indicating that a future has stopped executing one way or another (failure, cancellation, normal completion) and as a state meaning "completed successfully". Those two uses are in conflict.

The existing code is mostly threading / multiprocessing agnostic, but we're using threading.Event objects, for example. We should make it actually agnostic, and add tests with both threading and multiprocessing.

Even if there is no plan to write a WxPython version of qt_message_router any time soon, it might make sense to put some of the machinery in place to allow it.

The simplest approach for now would be to check that the value of ETSConfig.toolkit is either qt4 or qt before importing qt_message_router and throw a reasonable exception if not.

A slightly better approach would be to have an entry point for the message router keyed off the value of ETSConfig.toolkit which would at least allow 3rd parties to contribute their own message router.

EDIT [mdickinson, 2018-08-04; adding subtasks]

Subtasks:

• add entry-point-based mechanism for getting the router: #70
• add a wx message router: #72
• add tests for the wx message router (possibly not easy): #72
• make all tests run on wx: #72
• include wx in CI test runs: #72
• make progress example work on wx

We should have gh-pages documentation, so that docs.enthought.com/traits-futures works.

It's possible for the background task to fail with an exception after cancellation is requested. In that case, the end-state of the future is CANCELLED, and no "result" or "exception" attribute is present (for simplicity and consistency). Nevertheless, an exception occurring in the background job is something that should probably be reported somehow.

Consider logging a warning in this case.

Depends on #24.

The running and stopped Traits can't currently be listened to for changes, since they don't declare depends_on.

This isn't trivial to fix, because if we do add the depends_on, then those traits fire every time state changes, even if that state change doesn't imply a change in the derived state. Ideally, the traits would only fire when the derived state changes.

We should either fix this, or document that these traits shouldn't be listened to.

Related: #60

Currently we have to do something like this:

@on_trait_change('future:done')
def _do_something(self, done):
    if done:
        <do something>

It would be nicer (and safer) if we didn't have to include the if done line; that is, if a notification only gets fired when done changes from False to True.

Constraints:

• we don't want done to be an Event, since we want to be able to inspect it later
• we want done to remain consistent with state at all (observable) times.

The close button on the prime_counting example should cancel progress, but doesn't.

We need to set up continuous integration on this repository, ideally on all three of macOS, Linux and Windows.

This is less of an issue and more of a question that can hopefully be resolved easily.

I was looking in to the BackgroundProgress implementation, and saw this line:

Every progress submission also marks a point where the callable can
be cancelled.

Looks like that is handled by the ProgressReporter.report method.

I had a couple questions about how this would actually get used.

1. It seems like, once the task has started, it is no longer in a "cancellable" state, so the logic to cancel when reporting progress would never actually happen. Am I understanding this correctly? EDIT: It seems I was confused, EXECUTING is in the CANCELLABLE_STATES list.
2. Assuming the cancel would work on a task that is running but reporting progress, I would like to catch the _ProgressCancelled exception and clean up the current state of the task before re-raising the exception. Is this against what you would call "best practices" for this library? I usually shy away from accessing private methods/exceptions/classes, but it seems like this is the correct way to handle it.

Consider:

• providing submit_call and submit_iteration helpers on the TraitsExecutor (easy)
• having the TraitsExecutor automatically create a concurrent.futures.Executor instance when needed. For this, we need to figure out how to solve shutdown. (#42)

The message router for the TraitsExecutor is created on first use. If no jobs are submitted, that first use is at executor stop time: the message router is instantiated only to call its disconnect method. It would be better to rearrange the logic so that the message router is either always created at __init__ time, or not created at all if it's not used.

We have a hard-coded max_workers=4 where the TraitsExecutor creates its underlying concurrent.futures.ThreadPoolExecutor. That's wrong!

At the very least, that 4 should be a module-level constant, but it would be better to provide a way to create a TraitsExecutor that:

• owns its underlying executor
• allows the number of threads in that executor to be set in advance

The current code (currently in the unmerged PR #2) is almost Python 3 compatible, but there's at least one instance of import Queue as queue that needs fixing.

Using six is the way to go here.

See #2 (comment)

We could possibly use Python 3's built-in enumerations for the states of the futures and the executor.

On the plus side, this avoids users having to import lots of constants (CANCELLED, FAILED, COMPLETED, ...) from traits_futures.api. On the minus side, it's an extra non-std. lib. dependency on Python 2, and it's not a pattern we use much elsewhere (e.g., in ETS).

Noticed in passing, but don't have cycles to fix this minute: there are two occurrences of Attributerror in comments / docstrings in the codebase. Those should be AttributeError.

We have background calls and iterations. A variant that's useful is a long-running calculation that eventually yields a result, but also reports progress as it goes.

Currently our null backend has a custom-created event loop, for ease of testing. Now that we can depend on Python 3, it would make sense to replace that with something that uses asyncio.

The TraitsExecutor specialised submission methods submit_call, submit_iteration and submit_progress are a clear violation of the open-closed principle: we shouldn't need to modify the TraitsExecutor in order to add a new background job type.

We should deprecate these methods in favour of convenience functions submit_call, submit_iteration and submit_progress coming from the modules that define BackgroundCall, etc.

We should add debug-level logging throughout to make it easier to trace issues.

The example UI is currently very sluggish when there's work going on in a background thread. I'm having difficulty tracking down the cause, but the ui_dispatch function from Traits seems slower than it needs to be. We should be able to replace it with a simple Qt signal-slot mechanism (a single receiver QObject in the main thread).

There's a lot of duplication between the various background tasks. Some refactoring would be in order to remove that duplication.

At the same time, we should probably move the tasks into their own subpackage.

We should be using the Enthought style for the documentation, like other ETS projects.

Fix this before the 0.1.0 release.

The repository needs a README file, a setup file and all the other usual bits and pieces that go into a project.

To be useful, this repository needs tutorial-style documentation. As a first step, it might be enough to put that in the README.

We need to add tests that make use of the actual UI event loop.

Per InfoSec, we should apply a BSD license.

@dpinte Is there a preferred variant that we should use here, or should we just pick something?

We should add a copyright checker to ensure that all files have an appropriate copyright header. We may be able to simply steal the one that Traits is using.

A twine check on the generated source distribution fails due to a missing long_description.

When running the pi_iterations example, the button greyed-out status doesn't seem to act as I'd expect.

1. On initial launch, buttons are as expected: approximate is active, and cancel is greyed out.
2. After clicking "Approximate", the calculation starts as expected, but "cancel" remains greyed out until an application switch or a window resize. However, it does appear to be usable, despite the greyed-out appearance.
3. After clicking "Approximate", the button appears to be still active (but is actually inactive).

This may be a TraitsUI / Pyface issue.

The distinction between WAITING and EXECUTING isn't all that useful, and the states are potentially misleading:

WAITING means that the job may or may not have started executing
EXECUTING means that the job definitely has started executing (and may already have stopped)

There are few practical situations I can think of where it's useful to distinguish between the two states, and combining the states would remove a small amount of code and the need for an extra runtime message per job.

The proposal would be to drop the "WAITING" state: every job would start in "EXECUTING" state.

Now that an iteration can return a value, that makes it easy to break up an existing iterative calculation so that it can be cancelled easily. It would be good to document this possibility.

For example, a job submitted using submit_call looking like this:

def f(args):
    for i in range(1000):
        <do some work>
    return result

can now be submitted using submit_iteration instead, and written like this:

def f(args):
     for i in range(1000):
        <do some work>
        yield  # provide possible cancellation point
    return result