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

Readme rework draft #163

Merged
merged 11 commits into from
Feb 25, 2021
Merged

Readme rework draft #163

merged 11 commits into from
Feb 25, 2021

Conversation

goodboy
Copy link
Owner

@goodboy goodboy commented Oct 16, 2020

Check the rendered version.

TODO:

Defer to "branding" PR?:

Defer to docs rework PR (the readme should be simple)

- [ ] add some process tree diagrams using both something like mermaid.io and output from pstree? deferred to #157

README.rst Outdated
@@ -1,6 +1,7 @@
tractor
=======
A `structured concurrent`_, async-native "`actor model`_" built on trio_ and multiprocessing_.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks pretty good to me. If I were you, I'd want to decouple my pitch for tractor from multiprocessing, and leave that as an implementation detail.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@ryanhiebert it links to wikipedia not the stdlib; so it's in reference to actual muli-processing as an OS usage pattern.

Not sure if that was clear. Maybe multi-processing (with the dash) makes that more clear?

If you just mean that not focusing on it being implemented with multiple processes (since in theory we could somehow move to a threading system eventually?) then definitely worth considering.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I was thinking it was about the stdlib module. The dash would make more sense to me.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, the plan is to move entirely off the stdlib module asap.

The only thing blocking that is #146 for which I'd like to keep the stdlib stuff to compare against.

@goodboy goodboy changed the base branch from master to infect_asyncio October 16, 2020 17:55
@goodboy
Copy link
Owner Author

goodboy commented Oct 23, 2020

@ryanhiebert btw there's a matrix room now for the project if you feel like joining us.

@goodboy
Copy link
Owner Author

goodboy commented Nov 26, 2020

I think I figured out we can make a pretty simple "worker pool" example to address the first todo once #69 comes.

Something along the lines of a round-robin request-response pool; real simple.

Just sticking this comment as reminder for meself.

Copy link
Collaborator

@chrizzFTD chrizzFTD left a comment

Choose a reason for hiding this comment

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

Proposed README updates look good to me, thanks @goodboy !

I want to come back and try the examples and a bit more in the coming weeks on Windows 😃

try:
import uvloop
loop = uvloop.new_event_loop()
asyncio.set_event_loop(loop)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor, but the helper uvloop.install() exists. I'm unsure of any actual difference, though.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks v gtk.

README.rst Outdated
or simply as a replacement for the stdlib's `multiprocessing` but built
on async primitives for IPC.

``tractor``'s nurseries lets you spawn ``trio`` *"actors"*: new Python
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super nitpicky, but I think nurseries let you (instead of lets) is the correct grammar?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Pretty sure this is fixed now.

README.rst Outdated
-----------------
- Infinitely nesteable process trees
- A built-in API for inter-process streaming
- A (first ever?) "native" multi-core debugger for Python using `pdb++`_
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excited to try this soon!

Copy link
Owner Author

Choose a reason for hiding this comment

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

🥳

@goodboy
Copy link
Owner Author

goodboy commented Dec 9, 2020

Further things todo:

  • add some process tree diagrams using both something like mermaid.io and
  • the equivalent output from pstree -a $(pgrep <tractorprog>)
  • maybe a .gif of the above bullet?

EDIT

moving these to above list.

@goodboy goodboy force-pushed the readme_pump branch 3 times, most recently from 96daab2 to dd6271c Compare December 9, 2020 18:07
@goodboy
Copy link
Owner Author

goodboy commented Dec 9, 2020

For the worker pool example I'm thinking we might as well re-do the concurrent.futures prime number example so that peeps have a cross reference for what the insides of a ProcessPoolExecutor looks like implemented in tractor.

@goodboy
Copy link
Owner Author

goodboy commented Feb 24, 2021

Latest. hopefully final rework is up!

Copy link

@McSinyx McSinyx left a comment

Choose a reason for hiding this comment

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

Coming from the trio room on Matrix: I was wrong about the empty line after the headings though.

Comment on lines +38 to +40
.. code:: python

"""
Copy link

Choose a reason for hiding this comment

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

Per the reST specs,

Several constructs begin with a marker, and the body of the construct
must be indented relative to the marker.

where in this case, the code-block (I can't seem to find code even in Sphinx-flavored reST) body should be indented by 3 space. I believe that the editor picked this up and caused some indentation issues below.

Copy link
Owner Author

@goodboy goodboy Feb 24, 2021

Choose a reason for hiding this comment

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

Oh so you mean after the .. there should be 3 spaces before the code::?

Copy link

Choose a reason for hiding this comment

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

Yep, e.g.

.. code-block:: python

   """
   ...
   async def target():
       try:
   ...

BTW could you please hide the resolved reviews, my international connection is terrible due to cable maintenance or something and GitHub's way of fetching content really does not help 😄

docs/README.rst Outdated
Comment on lines 61 to 66
await trio.sleep_forever()


async def main():

async with tractor.open_nursery() as n:
Copy link

Choose a reason for hiding this comment

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

Like here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice catch. Fixing all this.

docs/README.rst Outdated
Comment on lines 78 to 79
if __name__ == '__main__':
try:
Copy link

Choose a reason for hiding this comment

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

Here as well.

@goodboy
Copy link
Owner Author

goodboy commented Feb 24, 2021

@guilledk yeah not sure what's going on with that worker pool link.
Code looks right now?

docs/README.rst Outdated
is ``tractor``'s IPC!


.. _concurrent_actors_primes: https://github.com/goodboy/tractor/blob/master/examples/parallelism/concurrent_actors_primes.py
Copy link
Owner Author

Choose a reason for hiding this comment

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

@guilledk not sure why it's not working?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This might be it:
https://sublime-and-sphinx-guide.readthedocs.io/en/latest/references.html#use-custom-link-text

Do not include a space between the last word of the link text and the opening angle bracket for the anchor text.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Lol nope nm.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah no idea. The code looks right -> seems to be some GH thing.

@goodboy goodboy force-pushed the readme_pump branch 2 times, most recently from 1626fae to 82ba680 Compare February 24, 2021 14:22
@goodboy goodboy mentioned this pull request Feb 24, 2021
13 tasks
@goodboy
Copy link
Owner Author

goodboy commented Feb 24, 2021

Lol, well turns out CI did in fact find a bug due to #185.

It appears that debug_mode=True isn't triggering crash handling in the root process only when not explicitly using the async with open_root_actor() entry-point; with the implicit runtime, errors caught in the root actor aren't being bubbled into the wrapping AsyncExitStack when using async with tractor.open_nursery().

I'm going to pull out the commit that introduced this change to the debugging example, move that history to a new branch + PR along with changes to port all the debugging examples to the new API.

@goodboy goodboy merged commit a0981be into master Feb 25, 2021
@goodboy goodboy deleted the readme_pump branch February 25, 2021 14:24
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.

None yet

6 participants