Make scenario finalizing more functional #65

kontsaki · 2021-01-23T20:41:00Z

Running a scenario now returns a finalizer handler
which can be used to wait until a timeout for each
running scenario to finish. The runner is not exposed
to the user anymore.

@goodboy would something like this make sense?

goodboy · 2021-01-24T16:15:25Z

README.md

@@ -106,20 +106,21 @@ You can also launch multiple multi-UA scenarios concurrently using
 non-blocking mode:

 ```python
-scens = []
+finalizers = []


Yah much better.

goodboy · 2021-01-24T16:24:25Z

pysipp/launch.py

@@ -185,3 +180,14 @@ async def finalize(runner, agents, timeout):
        raise SIPpFailure(msg)

    return cmds2procs
+
+
+def finalizer(finalize_coro, runner, agents):


So if we go with this I guess it's a breaking change right?
We might need a major version release in that case, which is cool, and probably not that cray since we already are cutting py2.

do you mean bumping to version 2.0 some weeks after version 1.0 was released? 😄

Hehe yeah kinda.

I have a feeling we're going to break the public API a lot with all the python 3 / async stuff.

I think anyone afraid of that should stick to the 0.1.0 (which i'm sure lots of peeps are using in production already) and then we can move forward making the api more functional and async in a 1.0

What you think?

Also so sorry for 👻 ing; it's been quite the week 😂

ah i just realized the release version you did a while back was 0.1.0 not 1.0.0

finalize_coro here is what?
If it's an async function i don't think we should have _coro.
Also in trio in general there's a distance kept from referring to coroutine objects directly so I'm thinking we can do a better name here?

oh yes, i think we don't even need to pass the finalize there, so we can remove the argument altogether.

Actually further, unless I'm missing something, I don't think this is going to work.

If you return the with_timeout closure then trio.run() isn't going to be invoked until the user calls finalizer() in which case the when block=False here:
https://github.com/SIPp/pysipp/pull/65/files#diff-2b80e3f62bb560133d08a5a07a74bc8eac811db3cdf2680136050b78f37d324dR161

there won't be any background code run.

I can't remember exactly if we're calling whatever run_all_agents() returns but if it does, we should change the name I think?

I've removed the argument.
I'm not sure what you mean, Scenario runs TrioRunner, through run_all_agents, which uses open_process

I'm confused too 😂

I don't get how this finalizer() can work.
Why does it need an inner closure like this?

Isn't trio.run() already running at this point, and if so why are we calling it again?

Maybe i'm forgetting how things work.

we trio.run once to run the subprocesses then once again to wait them to end or until timeout.

goodboy

The one thing I was going to question was the move to " over '.

I get that that's a common convention but I remembered recently reading the pydantic author saying he preferred the '.
pydantic/pydantic#595 (comment)

Also this SO shows that it shouldn't matter pep8 wise.

I'm wondering if I'm naturally inclined to just have to hold shift modifier less 😂

If you really want the double quotes though that's fine by me since I'm not actively working on this code base right now.

goodboy · 2021-01-30T19:51:34Z

@kontsaki yeah I think I don't really have a problem with the quotes change so much but having that here has made it pretty hard to read the functional changes in this PR - lotsa noise in the diff.

If you want to pull the quoting out into a separate formatting PR I think that would suit the git history better as well 😸

kontsaki · 2021-01-30T20:54:20Z

oh right, sorry, my editor auto formats with black on saving and black is pretty opinionated about formatting. I too tend to avoid hitting modifiers when i don't have too, but in this case i just use single quotes when developing and leave black do all the dirty work and in general i find double quotes easier to read. i will try to remake the PR with only the proper changes.

goodboy · 2021-01-30T21:45:01Z

but in this case i just use single quotes when developing and leave black do all the dirty work and in general i find double quotes easier to read.

Yah as mentioned I'm totally fine with the double quotes and we can probably add black to CI if you want but for understanding the details of this PR it would be handy to not have all that here 😂

goodboy · 2021-02-12T13:55:49Z

@kontsaki yeah just fire the smaller PR when ready and also the " change in a new one as well.

Also feel free to PR in black to CI if you want.

kontsaki · 2021-02-13T10:39:37Z

@goodboy i've replaced single quotes with double quotes, black does more formatting though, if this PR can't be merged as is i may come back at a later time when i have more time to remake everything.

kontsaki · 2021-02-13T10:47:08Z

i think it would be easier if we format all the project with black #67

goodboy · 2021-02-13T16:46:58Z

@kontsaki looks like you've still got the black formatting history here so I still can't really read the diff 😂

Is there any way you can adjust the history to show just functional changes by rebasing those commits out of your original branch and then making a PR with just those commits here?

It should be something like git rebase start_commit end_commit --onto drop_py27_more I think.

goodboy · 2021-02-13T16:48:43Z

Ahh I guess if it's your editor though that's going to be a pain eh since these changes are probably embedded in the commits themselves...

Maybe I'll just give it another go 🏄🏼

kontsaki · 2021-02-13T16:53:13Z

we can also just merge #67 into master and i will rebase so it will only show the related diff

kontsaki · 2021-02-13T18:07:19Z

after merging #69 the diff will be clear

kontsaki · 2021-03-21T19:22:28Z

hey @goodboy , any chance to merge this and #69 or should i close them?

goodboy · 2021-03-21T23:35:32Z

@kontsaki yeah of course!

Sorry just been caught up with other stuff.

I've commented in #69

kontsaki · 2021-05-27T14:03:47Z

ping :)

goodboy · 2021-05-28T18:58:11Z

@kontsaki heh. unless GH is being weird for me, seems like that didn't help at all 😂 (as I was kind of expecting).

goodboy · 2021-05-28T19:01:39Z

Yup so afaict this is exactly what I expected: we've now got a merge history (mess imo) and we aren't any closer to making it easy to review the functional changes here.

@kontsaki, i I think you should take this branch and rebase it onto drop_py27, work through all the conflicts again and then we can discuss.

goodboy · 2021-05-28T19:21:09Z

@kontsaki so as mentioned in #55 please factor out these black commits from here by rebasing this branch onto that one since I presume we're still keeping that one as the target?

kontsaki · 2021-05-28T19:41:38Z

hello, yes I will it a try!

Running a scenario now returns a finalizer handler which can be used to wait until a timeout for each running scenario to finish. The runner is not exposed to the user anymore.

This reverts commit 7d40d9c.

kontsaki · 2021-08-04T07:41:05Z

hey @goodboy i rebased like you've requested, this is clear now.

goodboy · 2021-08-04T15:00:01Z

README.md

-for scen in scens:
-  scen.finalize()
+for finalizer in finalizers:
+  finalizer(timeout=10)


I think we've talked about it in passing but, we might want to move to an explicit method name here. I'm not against .__call__() per say but it does seem a bit too implicit the more we've let this sit.

Definitely something for future work though, no need to address here.

hello, are you talking about the ScenarioType having both run and __call__ methods?
the finalizer returned is just a function. Docs are incorrect here thought i will fix them, calling scen returns the finalizer

I'm talking about 2 things:

Scenario.run() I think should be the api we encourage using.

you dropping .finalize() as part of the runner api.

Right now you've made things arguably more indirected by returning a closure from Scenario.__call__() only when block=False. I wouldn't call this any more functional per say other then that you return a closure instead of the runner.

The user should have access to the finalizer only when block=False. It would make much more sense if the finalizer was a method on the Scenario instead of juggling between objects, but that wasn't the case in the existing implementation.

It would make much more sense if the finalizer was a method on the Scenario instead of juggling between objects, but that wasn't the case in the existing implementation.

I like this idea better then returning an anon closure personally.
Is there any reason we can't add this?

It also would tie multi-script-execution lifetime to the scenario object, and for example if one wants to get the results from the last scenario run you just call Scenario.results() or Scenario.finalize() or wtv we decide. This makes the scen a little more future-like which it kind of is given we have non-blocking run support?

This solution will change the existing API and be backwards incompatible btw.

We are trying to implement a sync interface to an async functionality.

We can make the Scenario an awaitable to simplify the interface, but this would require the user to run async tests. Or the Scenario can just be a context manager.

This solution will change the existing API and be backwards incompatible btw.

Not if we make Scenario.__call__() a thing tho right? Also, right now we're documented as scen.finalize() as the api no? I'm not sure how we're breaking anything exactly?

We are trying to implement a sync interface to an async functionality.

Yes which is why this needs thorough thought and ideally more then just our opinions 😂

Scenario can just be a context manager.

This idea i don't mind. If we're going to offer a sync interface it may also make sense to consider spawning trio in another thread and then having the main thread make calls to it with trio.to_thread() and friends.

We should eventually be supporting async code for spawning and scenarios especially to support multi-script cases where a user wants to do cmd restarts, cancels, etc.

goodboy · 2021-08-04T15:16:51Z

pysipp/launch.py

+def finalizer(finalize_coro, runner, agents):
+    def with_timeout(timeout):
+        return trio.run(
+            partial(finalize_coro, timeout=timeout),


This partial also feels weird.
Maybe put the positional args inside it as well?

it's because trio.run passes only args and not keyword args but sure

goodboy · 2021-08-04T20:20:02Z

pysipp/launch.py

-        return runner
+            return await finalize(runner, agents, timeout)
+        else:
+            return finalizer(runner, agents)


Yeah, I just don't see how this is beneficial.

Instead of returning the runner now we're returning a closure that calls trio.run() and that closure seems mostly to be a function created solely for the purposes of having it be a function (aka an object with .__call__())

I actually find this somewhat hard to follow and not any more functional then the original method which does this same thing.

What is the benefit of exposing an implementation detail like the runner to the user? The downside is that it leaves more space for the user to misuse the API.

goodboy · 2021-08-04T20:23:13Z

pysipp/launch.py

@@ -178,3 +180,10 @@ async def finalize(runner, agents, timeout):
        raise SIPpFailure(msg)

    return cmds2procs
+
+
+def finalizer(runner, agents):


Yeah, I don't see how this gives anything more functional honestly.

Instead of a runner (which also defines .__call__() and thus is duck-typed like a function) you return a closure function (which could have just as easily with a partial(trio.run, partial(finalize, runner, agents, timeout=timeout)) which seems nearly to be a no-op indirection level).

What's the motivation?

the API is cleaner and less error prone as the runner is not exposed to the user anymore which means fewer chances of reaching invalid states. Functional as in less object oriented.

the API is cleaner and less error prone as the runner is not exposed to the user anymore which means fewer chances of reaching invalid states.

Not sure how that's possible to reach invalid states but yeah I guess in theory?

If we don't have a runner and only a simple function there there's also no future option for adding cancellation support, which is kinda what trio is all about. I don't want to over-design now or anything but making this the api is yes maybe simpler (as much as .__call__() vs. .finalize() is) but it may be somewhat limiting down the road?

Functional as in less object oriented.

Because we aren't returning a runner?
Calling TrioRunner.finalize() vs. lambda.__call__() i'm not really seeing as being any more or less objecty in python but 🤷🏼. I still don't understand why you need this function block, why not just return the partial on trio.run() if you wanted this; heck it could be a lambda?

goodboy · 2021-08-09T16:24:06Z

README.md

 print("Finalizing all scenarios and collecting results")
-for scen in scens:
-  scen.finalize()


Yeah not sure what you mean about "breaking the api", you're already doing that with this change afaict.. so you'll have to explain.

goodboy reviewed Jan 24, 2021

View reviewed changes

goodboy reviewed Jan 30, 2021

View reviewed changes

kontsaki closed this Feb 12, 2021

kontsaki reopened this Feb 13, 2021

goodboy mentioned this pull request Feb 13, 2021

Drop py2.7 and add trio support! #55

Closed

kontsaki added 3 commits August 4, 2021 09:37

Make scenario finalizing more functional

315b0b0

Running a scenario now returns a finalizer handler which can be used to wait until a timeout for each running scenario to finish. The runner is not exposed to the user anymore.

Undo black formatting quotes

46566a2

Revert "Undo black formatting quotes"

8a4fd21

This reverts commit 7d40d9c.

goodboy reviewed Aug 4, 2021

View reviewed changes

Fix README about finalizing scenarios

81a345b

goodboy reviewed Aug 4, 2021

View reviewed changes

Refactor scenario finalizing

7e72ab7

goodboy reviewed Aug 4, 2021

View reviewed changes

goodboy reviewed Aug 9, 2021

View reviewed changes

kontsaki closed this Sep 15, 2021

Make scenario finalizing more functional #65

Make scenario finalizing more functional #65

Uh oh!

Conversation

kontsaki commented Jan 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodboy Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kontsaki Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodboy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodboy commented Jan 30, 2021

Uh oh!

kontsaki commented Jan 30, 2021

Uh oh!

goodboy commented Jan 30, 2021

Uh oh!

goodboy commented Feb 12, 2021

Uh oh!

kontsaki commented Feb 13, 2021

Uh oh!

kontsaki commented Feb 13, 2021

Uh oh!

goodboy commented Feb 13, 2021

Uh oh!

goodboy commented Feb 13, 2021

Uh oh!

kontsaki commented Feb 13, 2021

Uh oh!

kontsaki commented Feb 13, 2021

Uh oh!

kontsaki commented Mar 21, 2021

Uh oh!

goodboy commented Mar 21, 2021

Uh oh!

kontsaki commented May 27, 2021

Uh oh!

goodboy commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goodboy commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goodboy commented May 28, 2021

Uh oh!

kontsaki commented May 28, 2021

Uh oh!

kontsaki commented Aug 4, 2021

Uh oh!

goodboy Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodboy Aug 4, 2021 •

edited

Loading

kontsaki Aug 4, 2021 •

edited

Loading

goodboy left a comment •

edited

Loading

goodboy commented May 28, 2021 •

edited

Loading

goodboy commented May 28, 2021 •

edited

Loading

goodboy Aug 4, 2021 •

edited

Loading

goodboy Aug 9, 2021 •

edited

Loading

kontsaki Aug 4, 2021 •

edited

Loading

goodboy Aug 4, 2021 •

edited

Loading