fix tox environment settings (and fix deprecation warnings currently blocking this fix) #1124
Comments
|
Wow am I reading that right that that's a use of a deprecated API from within the python standard library itself? 😞 |
|
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 Everywhere I have the new version. |
|
Looks like this is because virtualenv has an old embedded version of |
|
In case anyone's interested in an update on this: I don't know a good fix for the
Both of these are coming from |
|
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 For the pandas warning, would For |
|
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 tox-dev/tox#436 (comment) too). I'll give it a try in the next few days.
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.
My initial attempt that when I was first trying to get it working didn't pan out. But I'll try again. |
|
Hey Zac, I got
I also got pandas working without an extra environment change by saying in the top-level I did make an attempt at using (I found and commented on the existing issue for one of those, but honestly didn't feel like digging into the rest.)
And honestly, I'm becoming a little skeptical about whether keeping track of everyone's 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. 😄 |
|
🎉 to the pytest and pandas fixes - they look good to me.
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
|
|
Cool, thanks for pushing me to do the 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 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 (python27 only) error on import pandas (latest, 0.22) (filed issue)Issue: pandas-dev/pandas#19944 Ignored via They added this to the 0.23 milestone, and there's also a comment that it would be irrelevant as of 0.24. import pip (latest: 9.0.1) uses deprecated `imp` module (commented on relevant issue)Issue: pypa/pip#5013 Ignored via It might be good to file a new issue so this isn't burried under a title that sounds like it's only about Python 3.7. pip (latest: 9.0.1) using deprecated `readfp` (filed an issue)Issue: pypa/pip#5056 I'm pretty sure pip doesn't have an issue for this yet, but I had trouble constructing a minimal example so I haven't filed an issue. Ignored via setuptools (latest, 38.5.1) uses deprecated imp module (there's already an issue)Issue: pypa/setuptools#479 Ignored via We're on on 28.8.0 but it looks to me like this still happens in 38.5.1. I checked by pip installing the latest setuptools into the tox environment and running: (fixed in latest setuptools) DeprecationWarning: Flags not at the start of the expression build\/.*\Z(?ms)Ignored via pytest uses deprecated imp module (there's an existing issue)Issue: pytest-dev/pytest#1403 -- labeled "help wanted", which is cool Ignored via pytest uses deprecated `convert` (I filed an issue, closed now b/c the fix is already in the the branch for the next feature release)Issue: pytest-dev/pytest#3280 Ignored via import pandas tries to use deprecated pandas.json (filed an issue)Issue: https://github.com/pandas-dev/pandas/issues/19944hypothesis uses invalid escape sequence in docstring (fixed in my PR)Fixed in: #1149 I'm happy I at least caught one problem of ours! This was a real issue in our code unrelated to any other packages: |
|
Something else to think about with the article: Suggesting that people run with 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.) |
|
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 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.
#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. 😄
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 #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. |
When has “that’s not a good idea” ever stopped me? 😜
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! |
Fair enough - I'll definitely need to update setuptools for #1091, to store conditional dependencies in
Looks like pytest can do that - we'd just have to sprinkle |
Hmm, maybe keep what we have in #1149 then (once I get it passing CI, which shouldn't take too much more)? |
@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? |
|
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. |
|
Certainly don’t wait on my behalf, my bandwidth for OSS is fairly small right now. |
|
There's also joke2k/faker#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 Makes it tempting to use |
|
The place I'm currently a bit stuck is: In our The Pinning new versions of Possible paths forward:
Sorry 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. 😄 |
|
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: 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 😄 |
|
Hey @Zac-HD -- my interpretation of the comment about a multiline This matches my experience of trying to use it: The only workaround I see is the one suggested at the bottom of the original issue:
Does this seem right? (Edit to clarify: This would mean using |
|
Ah, I misread that then. should localise errors where they belong (though... I haven't tried this either). |
|
Nope, should've mentioned I tried that too: Got any more? 😄 |
|
Nope, |
|
So @Zac-HD, it looks like 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 But are we going to need the Even then, I'd love to hear what you think, though. |
|
Ugh. I suggest we make 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 |
|
👍 |
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 #1122 (comment)) 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:The text was updated successfully, but these errors were encountered: