Skip to content

Prevent param.version from failing when used alongside async frameworks#389

Merged
ceball merged 1 commit intoholoviz:masterfrom
julioasotodv:master
Apr 17, 2020
Merged

Prevent param.version from failing when used alongside async frameworks#389
ceball merged 1 commit intoholoviz:masterfrom
julioasotodv:master

Conversation

@julioasotodv
Copy link

In some weird environments, param's way of retrieving version information has many bugs (since it opens a subprocess to call git, for instance, which gives problems when Python processes are created with systems such as Supervisor or Gunicorn). This commit adds the option for the library to keep on going, even if the version cannot be correctly retreived.

It turns out that param.version is running (or at least trying to run) some git commands in order to retrieve the version. The way the git commands are launched is through subprocess.Popen() as you can see here:

def run_cmd(args, cwd=None):

And well, it turns out that subprocess.Popen() is nor particularly async-friendly when spawned from an os fork instruction (just like Gunicorn does). Apparently, one of the problems is that it misreports the return code for the launched command.

The only solution I can think of would be to slightly modify the run_cmd function, to make it read the return code as either 0 if everything runs correctly; or nonzero if either the actual returncode read by subprocess.Popen() is nonzero or if the stderr is something other than an empty byte array.

This PR does exactly that.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.237% when pulling 2907a71 on julioasotodv:master into 08c2d44 on holoviz:master.

@ceball
Copy link
Contributor

ceball commented Apr 13, 2020

Thanks for persevering with this fix!

There are no tests of param.version in param - the tests are in autover. So in theory this PR should go to autover and be dealt with there (with tests added for this fix) before being imported back to param.

However:

  • param.version is already different from autover/at least one PR must have been accepted on param before that modifies param.version without coming from autover.
  • autover has had no commit since 2018.
  • I don't see how this change makes anything worse, and you've already verified it fixes your problem.
  • You opened the original issue over a month ago.

So, I've set a calendar reminder for myself to hit merge on Friday at 18:00 BST, which I'll do if nobody objects.

@julioasotodv
Copy link
Author

julioasotodv commented Apr 13, 2020

@ceball Cool! However, I decided to create the same PR for autover, to see if tests complain: holoviz-dev/autover#59

Edit: apparently, tests are broken in autover, as I tried to revert changes and just leave the code as it was before the PR, and tests did not pass anyway with a weird diff error...

@jlstevens
Copy link
Contributor

jlstevens commented Apr 14, 2020

Thanks for the PR!

I've been thinking how best to fix this issue in a way that causes the least disruption and hassle. It is tricky so I haven't come to any conclusions yet. Regardless, I think the change you are suggesting makes sense to make things more robust when async is involved.

In other words, @ceball I think your plan to merge is safe.

@ceball
Copy link
Contributor

ceball commented Apr 16, 2020

I think your plan to merge is safe

Great, thanks.

I also think holoviz-dev/autover#59 in its current form shows that the addition does no harm.

@ceball
Copy link
Contributor

ceball commented Apr 17, 2020

I'm merging as promised (just a few hours late :) ).

Param's version.py file has already been modified here in param (two times maybe?) since last copied in from autover, so I'm just following the trend. (It doesn't seem like a great idea though. Maybe we need a better solution? As long as the "tricky autover tests" - i.e. testing it in different environments/scenarios - are not lost, I don't mind what is done.)

@ceball ceball merged commit d445d58 into holoviz:master Apr 17, 2020
@ceball
Copy link
Contributor

ceball commented Apr 17, 2020

(If autover is updated in a different way before the next param release, then great, we can replace this change if necessary before param's release. I don't know the next expected param or autover release dates, though.)

@julioasotodv
Copy link
Author

Thank you for the merge!

Just for you to know why this was useful, now bundling standalone Panel apps (no Python nor Internet required) is a reality: as a bundle that includes a Bokeh Server + other async apps.

For instance, I created here a PoC you can try and run in your machine. It is an Electron.js app that has got both a ASGI app + a Bokeh Server embedded, which makes it easy to share (for instance) Panel dashboards just as an .exe or OSX dmg self-executable file.

This is extremely useful to share HoloViz apps when running a Bokeh Server somewhere in the network is not an option (mostly due to stupid corporate network security concerns). It is just a matter of sharing the executable with whoever you want.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants