Skip to content

Feature/use python future #10

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

Closed
wants to merge 10 commits into from

Conversation

darkjh
Copy link

@darkjh darkjh commented Jul 25, 2014

For issue #9

It's easier than I thought. Comment please.

raise exceptions.ExecutionBlocked

def result(self, timeout=None):
with self._condition:
Copy link
Contributor

Choose a reason for hiding this comment

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

self._condition is specific to Python concurrent.futures implementation.

Copy link
Author

Choose a reason for hiding this comment

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

Yes but as we reuse concurrent.futures.Future, the usage of this conditional lock should be consistent, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that concurrent.futures.Future was not intended to be a base class as the module lacks an abstract base class. Hence it already comes with some implementation details like the threading.Condition used to wait until the completion of the task.

I suggest we add this missing abstract base class that comes with the _condition attribute.

Copy link
Author

Choose a reason for hiding this comment

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

or we just don't care about the _condition here in SwfFuture, they are not useful.

@ggreg
Copy link
Contributor

ggreg commented Jul 25, 2014

⚠️ Beware switching back from class properties to methods changes the interface of Future and breaks compatibility.

@ggreg
Copy link
Contributor

ggreg commented Aug 1, 2014

Let's add an abstract base class simpleflow.futures.AbstractFuture.

And then inherit from it inside the packages that provides the backend implementations such as:

  • Local synchronous and asynchronous future in simpleflow.local.futures: class Future(concurrent.futures, simpleflow.futures.AbstractFuture)
  • SWF in simpleflow.futures: class Future(simpleflow.futures.AbstractFuture)

In local, the multiple inheritance is required to enforce custom feature of simpleflow added to the genuine concurrent.futures.Future.

@darkjh
Copy link
Author

darkjh commented Aug 4, 2014

@ggreg updated like what we've discussed last friday.
It seems that we've an swf authentication error during Travis build.

@ggreg
Copy link
Contributor

ggreg commented Aug 4, 2014

@darkjh #6 will allow to run the tests on Travis without calling the SWF API.

@ggreg
Copy link
Contributor

ggreg commented Aug 4, 2014

Thanks for the update, I have not reviewed it yet.

@ggreg
Copy link
Contributor

ggreg commented Aug 6, 2014

👍

Thanks @darkjh! Simpleflow 0.2.3 is in testing. When it will be validated, I'll merge #17. You can then rebase this pull request and we will merge it.

@ggreg
Copy link
Contributor

ggreg commented Aug 6, 2014

It will allow to continue the work on #8.

@darkjh
Copy link
Author

darkjh commented Aug 7, 2014

Yay! Rebase et tests passed!

@ggreg
Copy link
Contributor

ggreg commented Aug 7, 2014

Great! Kudos @darkjh.

@darkjh
Copy link
Author

darkjh commented Nov 5, 2014

@ggreg it's the right time to merge this?

@darkjh darkjh force-pushed the feature/use-python-future branch from 651bf4a to 9a1e162 Compare March 11, 2015 13:26
@darkjh
Copy link
Author

darkjh commented Mar 11, 2015

@ggreg it seems now the default branch is master? Also I didn't understand the test fail under pypy ...

@darkjh
Copy link
Author

darkjh commented Mar 11, 2015

@ggreg all tests pass under pypy-2.4.0 on my machine.

@ggreg
Copy link
Contributor

ggreg commented Mar 11, 2015

Travis uses pypy 2.5.0. I need to make the test fail with pypy locally.

@darkjh
Copy link
Author

darkjh commented Mar 11, 2015

I found that the 'dummy' future is very difficult to handle in this case.
With this PR, we'll have different Future for different executor backend. So explicitly instantiate an Future in the workflow definition is problematic: in the executor agnostic workflow definition, we need to know which Future to instanciate ..
Let me know what you think on this @ggreg

@jbbarth jbbarth closed this Sep 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants