Prevent autover from failing when used alongside async frameworks #59

julioasotodv · 2020-04-13T12:05:38Z

Basically the same PR as the one I did in param.version: holoviz/param#389

However, according to @ceball autover has tests for that part of param, so we can test it here.

Fixes #62

ceball · 2020-04-13T18:04:50Z

Removed the change just to see if Travis is fooling me

Looks like the travis build of master has been failing since around late 2018 :(

jbednar · 2020-04-13T18:24:21Z

@jlstevens is looking into it.

ceball · 2020-04-13T18:55:20Z

@jlstevens, looks like the test that's failing (for "depends on autover" tests) fails because of one or more changes to pip over time, meaning something about how the project is set up is no longer correct.

ceball · 2020-04-14T11:48:33Z

Now it's failing at downloading miniconda.

Edit: Hmm. autover is using a very old version of pyctdev, from when it was still called pyct. And pyct(dev) hardcodes the miniconda url. And the miniconda download url has changed from continuum to anaconda.

ceball · 2020-04-14T12:38:43Z

@julioasotodv The existing tests are now fixed, so you could rebase/merge/whatever. Plus a test for your change, if easy. But if you do want to add a test, note that the test framework is a bit tricky, for multiple reasons; if you can suggest a simple test to reproduce the failure you are fixing, I might be able to say how/where to add it.

julioasotodv · 2020-04-14T14:45:40Z

Hi @ceball I believe tests are still broken, given that this commit: f97d80b

Makes the linter step in Travis complain: https://travis-ci.org/github/pyviz-dev/autover/jobs/674887606

ceball · 2020-04-14T18:36:11Z

This is showing the "bundled" example/test of autover has not been updated. We check autover works in various installation/usage scenarios, and that's one of them. So looks like you have to make changes twice...

ceball · 2020-04-14T19:01:05Z

Actually, looks like there are two "bundle examples" - so three places to update in total!

I guess the second bundle example is because at some point we discovered a problem with repository name being different from package name, or something like that.

I'm sure we could do better than requiring developers to update three files to get all the tests to pass, but it's a low traffic project...

(Sorry for the giant screenshot - on my phone!)

julioasotodv · 2020-04-14T19:14:34Z

@ceball no, it’s fine; don’t worry. I’ll try again in a couple of minutes

julioasotodv · 2020-04-14T19:58:28Z

Finally!

ceball · 2020-04-14T20:47:23Z

Is there a way we can easily test this change, to make sure it does not recur? I.e. how do we trigger the problem in the first place? If you were to paste:

a code fragment that triggers the problem
how you are using the library that triggers the problem (e.g. you installed a released package using pip? or you installed from a clone of the project's git repository via a pip editable install? or you installed via pip straight from github? or...?)
the bad result without the fix applied - an exception, or...?
the good result with the fix applied - just getting a __version__ string of "None" or something like that?

then I could probably add it as a test.

It would be great if you could open an issue on this repository with the above info - I've lost track of where the original report was (I'm sure there was one with more detail at some point...)!

Having said all that, I don't think it's necessary, in that I would merge the change here anyway (but @jlstevens will be the one to say what to do).

julioasotodv · 2020-04-15T03:08:29Z

@ceball I added a new unit test in the PR.

Basically, when autover is used in a Python process created by os.fork() (just like Gunicorn does, for instance), the run_cmd function returned an erroneous exit code when used alongside asyncio. Pretty corner case, but it was driving me crazy...

ceball · 2020-04-15T06:45:04Z

conda.recipe/meta.yaml

@@ -26,6 +26,7 @@ test:
  source_files:
    - tests
  commands:
+    - conda install -y -c conda-forge gunicorn uvicorn starlette


It should be possible to declare these dependencies rather than running a command explicitly: https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#test-requirements

Yes, I first thought about that. Unfortunately, apparently the test requirements cannot refer to a library that is not in the same conda channel as the project; and given that uvicorn is not available in the channel, it had no way of satisfying the requirement unless I explicitly conda installed it.

Ha ha, you can't win, sorry. No need to change then.

@jbednar @jlstevens can I change this project to use conda-forge?

.travis.yml

ceball · 2020-04-15T06:47:20Z

tests/test_asgi_server.py

+
+print(autover.__version__)
+
+app = Starlette()


Is this file a "test support file" rather than an actual test? If so, maybe rename it so it's clearly not a file containing tests, and then no need to exclude specially from nose? (Unless there's some reason that's not possible?)

I did it that way just to make sure that the file got bundled when creating the package (as everything that is named test* inside the tests folder does), but I can just modify setup.py to force the file inclusion there after renaming. I don't know which one is more elegant...

as everything that is named test* inside the tests folder does

Do you happen to know if that is a limitation we have put in somewhere here in autover? I wasn't aware of this as a general thing (in setup tools, tox, etc), and I'm only on my phone just now...

ceball

Without your fix, the test you added results in an exception? And with the fix, it results in the test passing - but could you add an assertion about what the value of __version__ is when it does pass? (I still don't understand what is being returned by autover for the version in this scenario, although I believe it's something like "None"? That seems wrong to me, but hugely preferable to the current situation, i.e. a crash...)

ceball · 2020-04-15T07:06:23Z

Thanks for all that work! I am impressed you figured out how to modify autover's code, how to contribute to autover plus its tests, and stuck with that process through the delays etc!

Pretty corner case, but it was driving me crazy...

autover in one sentence, ha ha!

But seriously, I believe the work you have done is important. autover causes various problems in real-world environments, and if we don't eliminate them, it's at best just embarrassing for projects like param, panel, holoviews, etc ("You want me to add a package to my production environment that runs git (multiple times!) every time a user imports it? Now why would I want to add a package that does stuff like that?"...).

julioasotodv · 2020-04-15T11:18:18Z

Without your fix, the test you added results in an exception? And with the fix, it results in the test passing - but could you add an assertion about what the value of __version__ is when it does pass? (I still don't understand what is being returned by autover for the version in this scenario, although I believe it's something like "None"? That seems wrong to me, but hugely preferable to the current situation, i.e. a crash...)

Indeed, the spawned process (for instance by Gunicorn) just chrashes with the following exception (without the fix):

File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/__init__.py", line 5, in <module>
    from . import layout # noqa
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/layout.py", line 21, in <module>
    from .io.model import hold
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/io/__init__.py", line 6, in <module>
    from .embed import embed_state # noqa
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/io/embed.py", line 21, in <module>
    from .model import add_to_doc, diff
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/io/model.py", line 14, in <module>
    from .state import state
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/panel/io/state.py", line 14, in <module>
    from pyviz_comms import CommManager as _CommManager
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/pyviz_comms/__init__.py", line 14, in <module>
    reponame="pyviz_comms"))
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 278, in __str__
    known_stale = self._known_stale()
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 223, in _known_stale
    commit = self.commit
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 133, in commit
    return self.fetch()._commit
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 162, in fetch
    self.git_fetch(cmd)
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 212, in git_fetch
    self._update_from_vcs(output)
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 258, in _update_from_vcs
    self._release = tuple(int(el) for el in dot_split)
  File "/home/julio/anaconda3/envs/bokeh_starlette_server/lib/python3.7/site-packages/param/version.py", line 258, in <genexpr>
    self._release = tuple(int(el) for el in dot_split)
ValueError: invalid literal for int() with base 10: ''

That's why in the tests we record what the spawned process logs, and we ensure its stderr is empty.

ceball · 2020-04-15T12:03:35Z

But what is the value of __version__ going to be now with your fix? (sorry that I don't know this, but I don't know autover's code...)

ceball · 2020-04-15T12:10:49Z

It looks like you print it, but I can't find the test in the travis job logs - at least, not on my phone 😂

julioasotodv · 2020-04-15T15:38:07Z

But what is the value of __version__ going to be now with your fix? (sorry that I don't know this, but I don't know autover's code...)

So, the issue came back from https://github.com/holoviz/param. Some other holoviz projects (such as HoloViews and Panel) make use of param.version in order to generate the version attribute for those libraries. And it turns out that param.version is an exact copy (AFAIK) of this project.

So basically the issue prevents me from using HoloViews/Panel in a async (asgi) web application, since the version generated attribute for those libraries was generated at runtime, and the runtime crashed.

Since param.version does not have any tests whatsoever, I decided to port the patch here in order to see that it does not break compatibility with the rest of param.version workflow.

As for why my test decides to print version, it is just a way to force its generation. It could very well be:

_ = autover.__version__

ceball · 2020-04-15T15:51:39Z

I'm only asking, what does it end up being set to in this scenario, after your fix? I realize it won't be the correct version, but what is it?

I'd like to put something like this in:

assert autover.__version__ is "None" # TODO this is not the correct version, but we are at least checking it does not crash; see https://github.com/pyviz-dev/autover/issues/62 for details.

I.e. I'm not asking for any more changes to the code, just to record what the situation is in the tests. Does it make sense?

Sorry for not being clear!

ceball · 2020-04-15T15:54:09Z

I now found the new test running on travis (I'm on a computer now :) ), but unfortunately the print is swallowed so I can't see for myself what __version__ is coming back as.

https://travis-ci.org/github/pyviz-dev/autover/jobs/675250008#L243

julioasotodv · 2020-04-15T15:55:00Z

I'm only asking, what does it end up being set to in this scenario, after your fix? I realize it won't be the correct version, but what is it?

I'd like to put something like this in:
assert autover.__version__ is "None" # TODO this is not the correct version, but we are at least checking it does not crash; see https://github.com/pyviz-dev/autover/issues/62 for details.
I.e. I'm not asking for any more changes to the code, just to record what the situation is in the tests. Does it make sense?

Sorry for not being clear!

No problem, it was just to give you some context.

The problem won't be that autover.version will become None, but rather that the whole Python Process will crash. The assertion in the test checks whether that Python process outputs stuff to its stderr before crashing or not. If there's nothing in that stderr, that means that there is no crash (the goal of the patch).

However, I will try to document the test and test_asgi_server.py to make it clearer

ceball · 2020-04-15T16:31:39Z

Sorry, we are misunderstanding each other here. I am asking you to say what the value of __version__ is after your fix. I.e. now it does not crash (great!!) but presumably the reported version is not correct and is just something weird like "None"? We should record that fact, is what I am getting at.

ceball · 2020-04-15T16:33:01Z

If you also have no idea what it is after your fix, no problem. (I just assumed you would know, because you have been running it.)

ceball · 2020-04-16T18:46:31Z

I got your PR and tried it on my own machine. If I run your test without your version.py code change, the test still passes - which it presumably should not do. I assume that is happening because the test is running in a git repo, and there is actually no trouble getting the version in such a scenario - right? And instead the test needs to mimic what would happen for an installed package? Or am I wrong? (If so, maybe it's down to a difference of platform/software versions?)

Anyway, early on, I asked for a few bits of info:

Is there a way we can easily test this change, to make sure it does not recur? I.e. how do we trigger the problem in the first place? If you were to paste:

a code fragment that triggers the problem

how you are using the library that triggers the problem (e.g. you installed a released package using pip? or you installed from a clone of the project's git repository via a pip editable install? or you installed via pip straight from github? or...?)

the bad result without the fix applied - an exception, or...?

the good result with the fix applied - just getting a __version__ string of "None" or something like that?

then I could probably add it as a test.

You have supplied the first one (the code fragment) - thanks!

I still need an answer to the second one (i.e. when do you get the problem? e.g. you pip install panel from pypi, and then when you use panel in your app, you get the crash?).

The third one (what happens without your fix?), I see you previously answered in a comment on your original pyviz_comms issue (thanks!).

The fourth one I can find out for myself if you answer the second one, above.

I can then move your test to the appropriate place, so that it has the right environment to trigger the problem in the first place.

It's also possible that I could convert your test into a simpler case that will work in a unit test, but I don't want to think about that until I can first actually reproduce your problem.

Thanks!

ceball · 2020-04-16T18:50:41Z

tests/testversion.py

+            with open(logfile_path, "r") as gunicorn_log_file:
+                stderr_log = gunicorn_log_file.read()
+
+            subprocess.Popen(["pkill", "-f", "gunicorn"])


I guess it would be better to kill exactly the pid that was created, in case there are other things running. But we might be able to create an entirely simpler test (we'll see once I can reproduce), so let's wait and see.

Yeah, I guess you are right. Fortunately, Gunicorn can output the PID that launches, so can be retreived by Python and be killed.

julioasotodv · 2020-04-16T22:38:35Z

I got your PR and tried it on my own machine. If I run your test without your version.py code change, the test still passes - which it presumably should not do. I assume that is happening because the test is running in a git repo

Exactly. The error happened either:

When the library comes from package without git repo (pip, conda...)
When git is unavailable

You can try it for instance if you temporarily add something like alias git=asdf to your ~/.bashrc, for instance, and then try to run the test.

With that said, when installed as package (with the patch), the __version__ is displayed correctly, as first the git command is issued, and if it fails (gracefully) it tries to get package info.

jlstevens · 2021-02-05T10:50:44Z

The changes to version.py look simple and safe to me so I am happy to merge as long as the tests go green. Right now I am seeing an error about uvloop missing so I've opened #67 to try and fix it (I would push to this PR but I don't have access to the fork).

jlstevens · 2021-02-05T11:31:52Z

In #67 the uvloop dependency is no longer complaining but test_run_asgi_server is failing: https://travis-ci.org/github/pyviz-dev/autover/jobs/757680271#L245

jlstevens · 2021-02-06T10:04:32Z

Tests are now passing in #67 after a rebase. @jbednar I am now in favor of merging that and closing this PR.

jbednar · 2021-02-15T16:43:19Z

Thanks for the PR! The underlying code change has now been merged as PR #67, so closing this version. The tests here haven't been merged, but they should no longer be necessary once we merge #66, since released packages won't be calling subprocess at all at that point.

julioasotodv mentioned this pull request Apr 13, 2020

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

Merged

ceball mentioned this pull request Apr 13, 2020

Guess at fixing failing build based on messages from pip. #60

Merged

julioasoto added 2 commits April 15, 2020 04:56

Prevent autover from failing when used alongside async frameworks

e1086e6

Added test for async server

d0a6733

ceball reviewed Apr 15, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

ceball reviewed Apr 15, 2020

View reviewed changes

ceball linked an issue Apr 15, 2020 that may be closed by this pull request

Crash while getting version when used alongside async frameworks #62

Open

This was referenced Apr 15, 2020

[Request] : Prevent git to be used for release packages #58

Open

Crash while getting version when used alongside async frameworks #62

Open

Removed dependencies installation from .travis.yml

fae9e7d

Better documentation for new test

1176fdf

ceball reviewed Apr 16, 2020

View reviewed changes

jlstevens mentioned this pull request Feb 5, 2021

Check stderr for subprocess call errors #67

Merged

jbednar closed this Feb 15, 2021


		print(autover.__version__)

		app = Starlette()

Prevent autover from failing when used alongside async frameworks #59

Prevent autover from failing when used alongside async frameworks #59

Conversation

julioasotodv commented Apr 13, 2020 • edited by ceball Loading

ceball commented Apr 13, 2020

jbednar commented Apr 13, 2020

ceball commented Apr 13, 2020

ceball commented Apr 14, 2020 • edited Loading

ceball commented Apr 14, 2020

julioasotodv commented Apr 14, 2020 • edited Loading

ceball commented Apr 14, 2020 • edited Loading

ceball commented Apr 14, 2020

julioasotodv commented Apr 14, 2020 • edited Loading

julioasotodv commented Apr 14, 2020

ceball commented Apr 14, 2020 • edited Loading

julioasotodv commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceball left a comment • edited Loading

Choose a reason for hiding this comment

ceball commented Apr 15, 2020

julioasotodv commented Apr 15, 2020 • edited Loading

ceball commented Apr 15, 2020

ceball commented Apr 15, 2020

julioasotodv commented Apr 15, 2020 • edited Loading

ceball commented Apr 15, 2020

ceball commented Apr 15, 2020

julioasotodv commented Apr 15, 2020 • edited Loading

ceball commented Apr 15, 2020

ceball commented Apr 15, 2020

ceball commented Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julioasotodv commented Apr 16, 2020 • edited Loading

jlstevens commented Feb 5, 2021 • edited Loading

jlstevens commented Feb 5, 2021

jlstevens commented Feb 6, 2021

jbednar commented Feb 15, 2021

julioasotodv commented Apr 13, 2020 •

edited by ceball

Loading

ceball commented Apr 14, 2020 •

edited

Loading

julioasotodv commented Apr 14, 2020 •

edited

Loading

ceball commented Apr 14, 2020 •

edited

Loading

julioasotodv commented Apr 14, 2020 •

edited

Loading

ceball commented Apr 14, 2020 •

edited

Loading

ceball left a comment •

edited

Loading

julioasotodv commented Apr 15, 2020 •

edited

Loading

julioasotodv commented Apr 15, 2020 •

edited

Loading

julioasotodv commented Apr 15, 2020 •

edited

Loading

ceball commented Apr 16, 2020 •

edited

Loading

julioasotodv commented Apr 16, 2020 •

edited

Loading

jlstevens commented Feb 5, 2021 •

edited

Loading