DRMacIver
commented
6 years ago Wow am I reading that right that that's a use of a deprecated API from within the python standard library itself? 😞
Closed dchudz closed 6 years ago
DRMacIver
commented
6 years ago Wow am I reading that right that that's a use of a deprecated API from within the python standard library itself? 😞
dchudz
commented
6 years ago Hmm, apparently that line was changed 7 years ago and I still have the old one when I run tox.
I'm not sure why I have the old version in .cache/hypothesis-build-runtimes/virtualenvs/tools-6d7a8f3c9c/lib/python3.6/site.py, or whether that's something weird that's unique to me. Sorry if I'm the one causing confusion here.
Everywhere I have the new version.
dchudz
commented
6 years ago Looks like this is because virtualenv has an old embedded version of site.py:
https://github.com/pypa/virtualenv/issues/555 & https://github.com/tox-dev/tox/issues/436.
dchudz
commented
6 years ago In case anyone's interested in an update on this:
I don't know a good fix for the site.py issue so I'm thinking of seeing if we can at least make FutureWarning an error. The current issues with that are:
import pandas fails (https://github.com/pandas-dev/pandas/issues/19944)test_timeout_traceback_is_hidden in pytest/ is a bit of an issue since it tests the output of a test that necessarily uses @settings(timeout=1) (which is deprecated) -- surely working around that is doable but seems a little awkward.Both of these are coming from basic-test.sh. One option is to surround both the pytest bit and the pandas bit with something like this:
export OLD_PYTHONWARNINGS=$PYTHONWARNINGS
export PYTHONWARNINGS="default"
$PYTEST --runpytest=subprocess tests/pytest
export PYTHONWARNINGS=$OLD_PYTHONWARNINGS
Zac-HD
commented
6 years ago I'll respond here instead of on the pull, just to keep the conversation in one place. (just mention if you'd rather move it)
It turns out that there is a fix for the site.py problem! tox-dev/tox#630 culminates in the publication of the tox-venv package, which drops support for Pythons 2.6 and 3.3 (who cares!) and fixes our issue here. Try it out, and if it works we should also leave feedback on that issue.
For the pandas warning, would PYTHONWARNINGS="default" $PYTEST --runpytest=subprocess tests/pandas work? That is, setting it for one command instead of exporting the var.
For test_timeout_traceback_is_hidden, I'd wrap the whole thing into another function and decorate that with @checks_deprecated_behaviour. This is indeed awkward, but should then be properly environment-independent.
dchudz
commented
6 years ago I guess I failed to read to the end the first time I saw that thread (thanks @Zac-HD for noticing it has a happy ending! I'm sure lots of other people will appreciate your update in https://github.com/tox-dev/tox/issues/436#issuecomment-369573251 too). I'll give it a try in the next few days.
would
PYTHONWARNINGS="default" $PYTEST --runpytest=subprocess tests/pandaswork?
Yes, sorry, that works for both is is actually what's in the PR. I got it wrong in the comment here but noticed the improvement before pushing.
I'd wrap the whole thing into another function and decorate that with
@checks_deprecated_behaviour.
My initial attempt that when I was first trying to get it working didn't pan out. But I'll try again.
dchudz
commented
6 years ago Hey Zac,
I got test_timeout_traceback_is_hidden working without messing with the environment: You can see in the PR, but:
@checks_deprecated_behaviour doesn't look easy to import from there (because the test is run from a random tempdir) so I used warnings.catch_warnings directly insteadwarnings.catch_warnings to surround both the test definition and the test execution -- a yield fixture seemed to be the way to achieve the latter, although I recognize it's getting pretty complicated-looking now. ☹️ I also got pandas working without an extra environment change by saying in the top-level setenv that we want to ignore FutureWarnings from pandas._version. I think that's better than what I had before.
I did make an attempt at using tox-venv to bypass virtualenv's DeprecationWarning. After various tracebacks from other stuff, I think this was enough to get make check-py36 working:
PYTHONWARNINGS={env:PYTHONWARNINGS:error::DeprecationWarning,default::DeprecationWarning:pip.pep425tags,default::DeprecationWarning:pip.wheel,default::DeprecationWarning:setuptools.depends,default::DeprecationWarning:setuptools.command.egg_info,default::DeprecationWarning:_pytest.assertion.rewrite,default::DeprecationWarning:_pytest.fixtures::FutureWarning}(I found and commented on the existing issue for one of those, but honestly didn't feel like digging into the rest.)
tox-venv itself worked great for Python 3.6 (all we need to do is add it to the requirements/tools.txt), but I didn't quite understand whether I could use it for our Python 2 builds (which still use virtualenv even with this).
And honestly, I'm becoming a little skeptical about whether keeping track of everyone's DeprecationWarnings makes sense for us. I'm especially loathe to create extra work for other people (you or David or whoever) who will end up dealing with more warnings that we have little control over down the line. I'm even a little worried about that with https://github.com/HypothesisWorks/hypothesis-python/pull/1149 (and won't be bothered by a suggestion that we shouldn't merge it), but it seems to be a much bigger deal with DeprecationWarning than FutureWarning.
Obligatory: I'm absolutely not criticizing the people, mostly volunteers, maintaining the libraries that call stuff with deprecation warnings. Just noting that the ecosystem is such that this could lead to a fair amount of low-benefit work for us right now.
I'm also not trying to deny that other packages' deprecation warnings could be an issue for our users. An upstream dependeny of ours that calls deprecated stuff can affect our users just as much as us calling deprecated stuff. I just think we are (or at least I am) much less likely to do something about those.
(I'm pretty open to helping solve it if you'd like us to, but this is where I've landed for now...)
It'd be pretty cool if there were a way to only get deprecation warnings coming directly from the package we're calling, and not ones that are just passed through the package we call. I might look around for something like that later, but haven't yet. 😄
Zac-HD
commented
6 years ago :tada: to the pytest and pandas fixes - they look good to me.
tox-venv just falls back to the standard system when you use it on Python 2, so we should be fine just using it unconditionally. Literally the only downsides are (a) you have to install it, and (b) it only supports non-EOL versions of Python - neither of which are problems for us.
And honestly, I'm becoming a little sceptical about whether keeping track of everyone's
DeprecationWarnings makes sense for us.
Fair enough! Part of my motivation here is that if we run CI with warnings as errors, it becomes easier for everyone else downstream - whether that's because we fixed or got something fixed, or just did the research.
I suspect that most of these deprecation warnings are because we're using old versions of pip, setuptools, and wheel. Proposed action:
dchudz
commented
6 years ago Cool, thanks for pushing me to do the DeprecationWarning bit too. 😄
I really like your idea for the post, and would be excited to do it. A couple thoughts:
On the actual PR (and as notes that might be interesting to your, or useful to refer back to when we write the post):
We're already using the latest pip (9.0.1) and wheel (0.30). One error would indeed be fixed by upgrading setuptools, but I chose to ignore it instead: I think (?) our setuptools version is coming from the default py36 tox environment, and if we want to change that I think we should do it in a separate PR.
Note that (I think) none of the external errors are warnings our users will get from using hypothesis - they're just stuff in our build/test pipeline. All of these came from running make check-py36 without the relevant ignore. Here's some more detail about the various errors (I've also put links to issues in the comments of tox.ini):
dchudz
commented
6 years ago Something else to think about with the article:
Suggesting that people run with -Werror in CI seems a little like asking volunteer open source contributors to do more free work. (In most cases I wouldn't actually suggest that internal corporate code, or anyone else fairly far downstream in the dependency graph, do this.)
I think there's a good article to write that doesn't isn't problematic in that way, but it does take some care, I think.
(Edit: Or maybe this isn't a big deal. Any suggestion about how to write better software is a bit like this, so I think as long as I don't write it in a way that doesn't sound like I think I'm entitled to have other people check for warnings in CI, then we're fine.)
alexwlchan
commented
6 years ago I would very much like such a post to exist, because I’m currently having this discussion at work and I’ve been unable to convince myself that it’s definitely a good idea!
Some brief comments:
I think the value of -Werror depends on your language. Compiled languages like C or Scala probably see more benefit than Python.
It’s the sort of thing that’s much easier if you turn it on at the start of your project, than trying to retrofit later. And sometimes even that’s not enough – witness shenanigans above with site.py.
I just went through the process of retrofitting -Werror with a year-old Scala repository (see https://github.com/wellcometrust/platform/pull/1615). I found a few deprecations, one naming confusion, and some other small things – but nothing especially serious or bad.
(Turning this on by fiat was somewhat naughty, and it's not guaranteed we won't revert the change later!)
My feeling is that this works best if the entire ecosystem buys into it. Otherwise single projects will end up swimming against errors in upstream.
dchudz
commented
6 years ago @alexwlchan:
I don't know the details of your work, but I'd be surprised if it's a good idea. 😄 I would not encourage anyone to try this at my work.
easier if you turn it on at the start of your project
https://github.com/HypothesisWorks/hypothesis-python/pull/1149 was indeed a fair amount work, but I think it gets us to where we'd be if we had this from the beginning. 😄
best if the entire ecosystem buys into it
Absolutely. Until then (and as the only way to get there), I think it's more sensible the further upstream a project is. I do think we probably are relatively far upstream in that sense (although less so than pip, distutils, setuptools, pytest, ....).
Are you concerned it might not be a good idea for hypothesis / are you opposed to https://github.com/HypothesisWorks/hypothesis-python/pull/1149?
As I mentioned above, a middle ground could be doing this in a way that applies this to only the tests themselves, and avoids the warnings from the build tools. That would have avoided about half the issues I found (although it feels like more than that -- I think the build tools ones were more work to e.g. report with a minimal test case).
I guess I'd prefer to go with what we have now, but I'm open to reworking the PR to something more like that, if that's the consensus.
alexwlchan
commented
6 years ago I don't know the details of your work, but I'd be surprised if it's a good idea. 😄 I would not encourage anyone to try this at my work.
When has “that’s not a good idea” ever stopped me? 😜
Are you concerned it might not be a good idea for hypothesis / are you opposed to #1149?
I’ll be honest, I haven’t had much time to keep up with hypothesis lately, so I haven’t read much of the surrounding discussion. I just caught mention of a possible blog post in the slew of emails, and jumped in!
Zac-HD
commented
6 years ago One error would indeed be fixed by upgrading setuptools, but I chose to ignore it instead: I think (?) our setuptools version is coming from the default py36 tox environment, and if we want to change that I think we should do it in a separate PR.
Fair enough - I'll definitely need to update setuptools for #1091, to store conditional dependencies in install_requires. Happy to leave it until then.
It'd be pretty cool if there were a way to only get deprecation warnings coming directly from the package we're calling, and not ones that are just passed through the package we call.
Looks like pytest can do that - we'd just have to sprinkle python -m -Werror pytest -Werror ... through tox.ini. Unfortunately we can't just put the incantation in an env variable, because the Windows syntax is different. Note also that this only takes effect after warnings filters, so we'd still need some environment tweaks or the double-Werror above (ie we'd dodge build and install issues, but not pytest issues).
dchudz
commented
6 years ago Looks like pytest can do that ... Unfortunately
Hmm, maybe keep what we have in https://github.com/HypothesisWorks/hypothesis-python/pull/1149 then (once I get it passing CI, which shouldn't take too much more)?
dchudz
commented
6 years ago I’ll be honest, I haven’t had much time to keep up with hypothesis lately,
@alexwlchan No problem, I just wanted to give you the chance to object if you wanted to. If you'll want to think about it in the next week or so, we could also hold off for that?
dchudz
commented
6 years ago The downside here isn't that large I suppose. If we ever want to give up on this, it's easy to take it out.
alexwlchan
commented
6 years ago Certainly don’t wait on my behalf, my bandwidth for OSS is fairly small right now.
dchudz
commented
6 years ago There's also https://github.com/joke2k/faker/pull/718. This one's a bit frustrating because (I think) it's possible to ignore the way the others can be ignored, since it ends up raising SyntaxError.
Makes it tempting to use pytest -Werror (or the [pytest] section in tox.ini) since I think that would ignore this kind of thing, but that would make me said because it wouldn't have caught the one issue we had in our own code.
dchudz
commented
6 years ago The place I'm currently a bit stuck is: In our py34 and py35 environments, using tox-venv gets us very old versions of pip and setuptools.
The py36 environment has new versions, and so do py34 and py35 unless we use tox-env.
Pinning new versions of pip and setuptools in requirements/test.txt makes this problem go away (make check-py34 passes), but pip-compile refuses to transfer those requirements from .in to .txt, telling me it's dangerous to pin those packages.
Possible paths forward:
pip and setuptools that we do, and fixing ittox-venv for those environmentsSorry this has been so long a process. @Zac-HD, if you're tired of it, feel free to let me know and I'll stop. 😄
Zac-HD
commented
6 years ago I think "fixing it" (ie installing the latest versions of pip, setuptools, and wheel) is the right way to go. We can (see comment and docs) use a multi-line install command for that:
install_command =
pip install --upgrade pip setuptools wheel
pip install {opts} {packages}I'll need to do this for #1091 anyway, but very happy for you to beat me to it! Finally - I'm happy to keep going for as long as you are :smile:
dchudz
commented
6 years ago Hey @Zac-HD -- my interpretation of the comment about a multiline install_command is just that it's asking whether that would address the issue if we had it. Not that we have it already.
This matches my experience of trying to use it:
tox.ConfigError: ConfigError: 'install_command' must contain '{packages}' substitutionThe only workaround I see is the one suggested at the bottom of the original issue:
Considering that doing the installation manually, using
commands =works
Does this seem right?
(Edit to clarify: This would mean using skip_install=True, and putting all installations in the commdands = section.)
Zac-HD
commented
6 years ago Ah, I misread that then. skip_install isn't my preferred workaround though:
install_command = pip install --upgrade pip setuptools wheel && pip install {opts} {packages}should localise errors where they belong (though... I haven't tried this either).
dchudz
commented
6 years ago Nope, should've mentioned I tried that too:
cmdargs: ['/Users/davidchudzicki/.cache/hypothesis-build-runtimes/.tox/py34-full/bin/pip', 'install', '--upgrade', 'pip', 'setuptools', 'wheel', '&&', 'pip', 'install', '/Users/davidchudzicki/.cache/hypothesis-build-runtimes/.tox/dist/hypothesis-3.47.0.zip']
You are using pip version 6.0.8, however version 9.0.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
Exception:
Traceback (most recent call last):
File "/Users/davidchudzicki/.cache/hypothesis-build-runtimes/.tox/py34-full/lib/python3.4/site-packages/pip/basecommand.py", line 232, in main
status = self.run(options, args)
File "/Users/davidchudzicki/.cache/hypothesis-build-runtimes/.tox/py34-full/lib/python3.4/site-packages/pip/commands/install.py", line 305, in run
name, None, isolated=options.isolated_mode,
File "/Users/davidchudzicki/.cache/hypothesis-build-runtimes/.tox/py34-full/lib/python3.4/site-packages/pip/req/req_install.py", line 181, in from_line
isolated=isolated)
File "/Users/davidchudzicki/.cache/hypothesis-build-runtimes/.tox/py34-full/lib/python3.4/site-packages/pip/req/req_install.py", line 54, in __init__
req = pkg_resources.Requirement.parse(req)
File "/Users/davidchudzicki/.cache/hypothesis-build-runtimes/.tox/py34-full/lib/python3.4/site-packages/pip/_vendor/pkg_resources/__init__.py", line 2873, in parse
reqs = list(parse_requirements(s))
File "/Users/davidchudzicki/.cache/hypothesis-build-runtimes/.tox/py34-full/lib/python3.4/site-packages/pip/_vendor/pkg_resources/__init__.py", line 2807, in parse_requirements
raise ValueError("Missing distribution spec", line)
ValueError: ('Missing distribution spec', '&&')Got any more? 😄
Zac-HD
commented
6 years ago Nope, skip_install it is then, unless the Tox issue turns up something useful :disappointed:
dchudz
commented
6 years ago So @Zac-HD, it looks like skip_install may require some large-ish changes in tox.ini. At least some of the current failures on https://github.com/HypothesisWorks/hypothesis-python/pull/1149/commits/c01b5cb8a90c6278b5462a9792d6adf56b0cdaef (e.g. ModuleNotFoundError: No module named 'hypothesis' in pytest28) are because the installs I added to [testenv] don't apply to the environments that define their own commands. So I think we'd have to add installs to all of those too, or something similar.
That seems like it could have all sorts of additional maintenance burden, and doesn't seem worth it just for this, especially because we can really get pretty much what we want by just running basic-test.sh with the environment variables we want after everything is installed.
But are we going to need the skip_install approach for https://github.com/HypothesisWorks/hypothesis-python/issues/1091 anyway? That might change things a bit, but I don't know enough about the problem in https://github.com/HypothesisWorks/hypothesis-python/issues/1091 to know what we'll end up needing.
Even then, PYTHONWARNINGS=... at the tox level has caused so much trouble that I think I'm inclined toward closing https://github.com/HypothesisWorks/hypothesis-python/pull/1149/commits and switching to a new PR that only sets PYTHONWARNINGS=... in basic-test.sh (maybe only in the $PYTEST --runpytest=subprocess tests/pytest line of it, too).
I'd love to hear what you think, though.
Zac-HD
commented
6 years ago Ugh. I suggest we make passenv, setenv, and FutureWarning work as intended, then leave the rest until later.
This saga is dragging out rather badly, and I'd rather get a decent solution in place now, fix #1091 (which may or may not involve skip_install, I'll find out when I do it), and then we can revisit after Hypothesis 4.0 is released and things have been simplified a little (for example, it's removing the Faker extra).
dchudz
commented
6 years ago 👍
The
passenvandsetenvin the[tox]section at the top oftox.iniaren't doing what's intended (they aren't doing anything, I think).The intention (as stated by @Zac-HD in https://github.com/HypothesisWorks/hypothesis-python/pull/1122#issuecomment-366567315) is:
Based on http://tox.readthedocs.io/en/latest/example/pytest.html and my experiments, it's easy to achieve that intention: Just move this stuff to the
[testenv]section a few lines below.However, if we do that currently, then a user running tox with
PYTHONWARNINGSunset won't even get as far as running any tests due to the deprecation warning: