-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Install lxml in optional dependencies on Travis #17936
Conversation
✅ Hi, I am the SymPy bot (v149). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.
Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. Click here to see the pull request description that was parsed.
|
5091165
to
82b62e1
Compare
Codecov Report
@@ Coverage Diff @@
## master #17936 +/- ##
============================================
+ Coverage 68.65% 74.792% +6.141%
============================================
Files 638 638
Lines 166336 166336
Branches 39109 39109
============================================
+ Hits 114191 124407 +10216
+ Misses 46601 36405 -10196
+ Partials 5544 5524 -20 |
The Side question: why does that module use |
We should also add this to the release script. I don't know if there's a good way to keep those in sync. |
Only on 2.7. They are not skipped for 3.6. |
This looks like it is being skipped on 3.6 https://travis-ci.org/sympy/sympy/jobs/614778623#L1270 |
I guess there's a more serious issue, which is that the "Python 3" optional dependency build on Travis is installing Python 2! We should add |
There are now 3 optional dependency builds. One is in the baseline tests - that's the 3.6 I'm referring to. |
Although it actually seems to be 3.7 |
Oh I see the one I was looking at is just for tensorflow<2. In that case we should fix it so that it
Even if it doesn't use the Travis Python we should make the version match because that's what it shows in the table, and it is very confusing otherwise. Actually that build should test Python 3 anyway, if it is possible. We are dropping Python 2 so if tensorflow < 2 only works in Python 2 then we will have to drop support for it. |
Yeah, we have it set to install that newest version of Python that it can. Previously it was installing Python 3.6 because there was some issue with the tensorflow package (conda-forge/tensorflow-feedstock#98). I guess that's been fixed. Eventually it will install 3.8 once all the packages we install support it. We should update the Travis metadata so it isn't confusing. |
There's a comment in .travis.yml that says it uses 3.6 because it's a faster container. But I don't think that is actually true. If you look at https://travis-ci.org/sympy/sympy/builds/614778607, for the 1/2 split the 3.8 build finished faster than 2.7 or 3.5, which use the faster container. And even if it were a little slower, I think it's better to have the version in the table match what is actually used. |
The I did try to get doctestplus to support something like sympy's doctest_depends_on but I can't remember where that discussion is now. Longer term I still think we should switch to pytest. |
Can SymPy's doctest_depends_on set that variable in the function module's globals()? Seems like it would be straightforward to make that work, so we don't have to duplicate it, unless doctestplus is reading the variable in some weird way. |
There are many possible ways to make this work. I think the real issue is: where do we want to end up long term? Do we want to maintain our own fork of pytest in perpetuity? The doctest_depends_on decorator is useful but there are many other ways to do the same thing. |
I think the decorator is a better way to do it, because it puts it right next to the docstring. Switching to pytest is a separate issue. Right now we support it, which is fine, but we should try to not have too many things duplicated because of it. To switch to pytest, we need to make sure it can support everything we do with our current runner. For the doctests I think the biggest issue is that the default runner doesn't run in a clean namespace, so the functions don't have to be imported. Does doctestplus allow you to change that? I'm sure there a dozens of other little things as well, like the depends on thing, which we would need to make sure are supported. For now we support pytest things by adding stuff to our own functions that make it work with pytest, so we should do the same thing with doctest_depends_on. If we end up switching to pytest only we can remove our custom things in favor of just the pytest things, or if our things are more useful than the pytest ones we can keep them. I'm not a huge fan of pytest myself, but it's really up the community what we want to do here. |
I think that the decorator is better because it works on a per-test rather than per-module basis. Otherwise I don't like the fact that the decorator is right there next to the function distracting attention from what the code should really be doing :) |
I think that for this PR it is fine that this only works for the Python 3.7 and tensorflow >= 2 optional dependency tests. This is a PR for master (because master already had changes to travis.yml) so I won't self merge it. |
If you don't want to fix the other Travis issues I mentioned in this PR that's fine. You can merge it as-is and open a new issue for them. I didn't merge it because I didn't know if you were going to address those things or not. |
The other issues I see are:
I can fix each of those. I see now that the second one is just the same change lower down... |
Actually I didn't even consider making the mathml tests run in 2.7. There's no point to doing that if this is only going in master, is there? The issues I saw were
Again, these are all unrelated so they can be done in separate PRs. |
I just tested |
I don't know enough about the changes in tensorflow to know what is right here.
I've just pushed that change here
I think I need to revisit this with the doctestplus people. It's a while since I looked at that and I can't remember what was said before any more... |
I made a PR for the tensorflow thing #17946 |
To clarify, this wouldn't be a change in doctestplus. I just mean having our decorator set the |
I realise that but if doctestplus can be improved to make it more suitable then that's better. |
It looks like changing the name of the optional dependency build to 3.7 breaks it: |
# This is actually 3.7 (once all the dependencies support it). We use | ||
# python: 3.6 here because 3.7 requires the slower xenial/sudo: true container, | ||
# and we aren't actually using the Travis Python anyway. | ||
- python: 3.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to add dist: xenial
here
References to other Issues or PRs
Partial fix for #8446
Brief description of what is fixed or changed
Adds lxml in the optional dependency tests.
NO ENTRY