pysipp icon indicating copy to clipboard operation
pysipp copied to clipboard

Drop py2.7 and add trio support!

Open goodboy opened this issue 6 years ago • 20 comments

This is in preparation of a python 3.6+ only 1.0.0.alpha release and replaces all the internal processing launching machinery with the new subprocess support in trio.

This not only gets us cross OS support for free but also gives access to async/await based scenario and agent spawning.

A few interesting notes:

  • SIPp turns out to be a good fit with trio cancellation semantics since it has a built in cancellation system via SIGUSR1
  • this now gives us a strictly single threaded implementation
  • trio gives us deterministic teardown allowing for much finer control over per agent failures and subsequent reporting

TODO:

  • write some async spawning tests and examples in the readme
  • consider playing around with timeout and other cancellation tools such as trio.move_on_after() and trio.fail_after() and figure out how this can interplay with log reporting for interactive use
  • add the mac os env to CI

I look forward to feedback and thoughts!

ping @y-luis

goodboy avatar Jul 11 '19 02:07 goodboy

Turns out we need to wait for the next version trio to be released before the subprocess stuff lands.

goodboy avatar Jul 11 '19 18:07 goodboy

Back to only one test failing; the one for async launching which needs a rewrite and new docs for how to do with this trio.

goodboy avatar Jul 12 '19 21:07 goodboy

hello, is there anything we can do to help merge this PR?

kontsaki avatar Dec 24 '20 10:12 kontsaki

@kontsaki my bad, I've let this slip quite a while 😿

I'm not currently working in telephony any more so haven't had any dire needs for this enhancement.

is there anything we can do to help merge this PR?

Testing and user feedback on the ergonomics would be good.

We were supposed to do a proper release a while back. I can't remember what the hold up was (it could have just been me?).

It would be good to get some further user feedback here on this though.

goodboy avatar Dec 24 '20 13:12 goodboy

@goodboy want me to open a PR for this branch if i get the chance and fix what you have commented?

kontsaki avatar Dec 24 '20 16:12 kontsaki

want me to open a PR for this branch

@kontsaki yeah 💯 that'd be amazing

We can use this branch to make some effort towards modernizing the codebase for #62 as well 🏄🏼

Actually just let me rebase this onto master first before you get started.

goodboy avatar Dec 24 '20 16:12 goodboy

@kontsaki there you go.

goodboy avatar Dec 24 '20 16:12 goodboy

@goodboy is it okay if i fix some pylint warnings and/or run the black formatter ?

kontsaki avatar Dec 25 '20 13:12 kontsaki

is it okay if i fix some pylint warnings and/or run the black formatter

@kontsaki do it in another PR if you want. Try to keep this one focussed around the new process launching and trio.

goodboy avatar Dec 25 '20 16:12 goodboy

@kontsaki I might just get this onto master as a new dev release so that interested peeps can try it out.

Just need to tweak the release number.. Also would be good to get a changelog going proper.

goodboy avatar Jan 14 '21 16:01 goodboy

Welp merged #67 and now the rebase on this branch will be a nightmare 😂

Not sure I'll get to it today.

@kontsaki unfortunately that means #65 can't really be simplified yet either until this branch is fixed.

This is why i normally don't do big formatting changes to someone else's code base when trying to get new features in 😉

We'll get through it eventually.

goodboy avatar Feb 13 '21 17:02 goodboy

i will fix the conflicts

kontsaki avatar Feb 13 '21 17:02 kontsaki

@kontsaki try doing a git rebase master from your copy of this branch you'll see what I mean.

There's quite a few tweaks that are needed and afaik it can't be done wholesale using a --ours flag.

goodboy avatar Feb 13 '21 18:02 goodboy

@kontsaki ok ur black changes should be in here now so let's take a look at #65 🥳

goodboy avatar May 28 '21 18:05 goodboy

Yah so this merge commit is not only imo messy it makes the history more confusing for onlookers.

@kontsaki take a read of this https://stackoverflow.com/a/13194092

The main reason i would require this is not just for (as some people think is the only benefit),

The only benefit of rebase strategy is linear history and that's it.

If mainline is desynced by a few commits it's nbd, but it's when you're making changes to every file in the repo (i.e. in the case of #69) it's much easier to manage the history and see the origin of broad changes if those changes are isolated in a linear path of the git tree.

goodboy avatar May 28 '21 19:05 goodboy

screenshot-2021-05-28_15-09-54

Now there are paths both to master and away from master - this imo is confusing especially if it gets repeated recursively. For example now if you don't rebase #65, and instead merge, we'll have yet another path going back out again which creates history cycles and then becomes harder to follow by manually inspecting commit history.

goodboy avatar May 28 '21 19:05 goodboy

i use a git alias: alias.logtree=log --graph --oneline --decorate --all which if you look at the repo history you'll see what i mean about avoiding away from mainline cycles in the history.

This (in imo) the cleanliness of rebasing and it comes at really no extra cost except forcing you to go over changes on your branch and potentially clean up your commits.

Can be done nicely with git rebase -i in your editor as well 😉

goodboy avatar May 28 '21 19:05 goodboy

Before we merge this I'd like to get some feedback from any users that are willing to alpha test it as well.

I don't think there's much risk bringing it into master, but also don't want to break anyone's setups who are pulling from git (since we used to not have a pypi package not that long ago). There's going to be some trio related version requirements for example I'd imagine.

If we get silence or feedback that everything is fine, we'll just merge and figure out peeps problems incrementally.

goodboy avatar May 28 '21 19:05 goodboy

@kontsaki are you happy with this patch set?

I see you closed #65, which actually I think is better for right now until we can get better idea of what runner api everyone prefers. I mentioned this a bit in the comments on that PR:

goodboy avatar Sep 15 '21 16:09 goodboy

I'm curious to know if anyone has actually used this branch for real testing; that would make it much easier to merge and move on.

goodboy avatar Sep 15 '21 16:09 goodboy

@goodboy are you willing to rebase this PR?

linuxmaniac avatar Dec 05 '22 07:12 linuxmaniac