[WIP] Migrate to pytest #6729
[WIP] Migrate to pytest #6729
Conversation
|
I'm not sure I'll find the time to review this. Theano isn't maintained anymore. Why do you want to do that for a software not maintained? |
|
You're right that this is a ton of changes for a project that is not actively developed. It may make more sense to do a hard fork here: My goal is to get at least 2 more years of stability for PyMC3: I think we gain a lot through a few tasks that are mostly automated
Do you have an opinion on the idea of a fork? I of course love Theano and appreciate all your work, but it seems as though your goals are to keep a lightly maintained version of Theano for legacy users, while PyMC3 is perhaps more willing to sacrifice backwards compatibility (and current stability! we certainly lack your familiarity with the codebase) for some amount of iteration. |
|
I totally understand wanting to support more recent dependency version. I try to review the PR/issues related to that. For me this is the most important point.
As you probably saw, we do not support the latest NumPy well. Someone started to work on that in PRs, but I think it stopped as I took too long to respond.
But why do you want to drop older supported dependency version? Normally we do that when we want to support more recent feature of dependency that aren’t available in the older version.
Which one do you want to drop and for which feature?
But what is your motivation for the 2 other points? I only see this as taking time to do and I do not see the gains.
Yes, it could lower Theano code base size, but if we do not change it, we do not care of its size.
For pytest vs nose, do you really need the features that pytests have vs nose tests? Your PR is touching practically all files.
It is easy to break something by mistake with such type of PR. So the risk of merging this is significant.
Before talking about a fork, I think we should try to find a common plan. This would be much better for everybody.
From: Colin <notifications@github.com>
Sent: Friday, November 8, 2019 9:16 AM
To: Theano/Theano <Theano@noreply.github.com>
Cc: Frederic Bastien <fbastien@nvidia.com>; Comment <comment@noreply.github.com>
Subject: Re: [Theano/Theano] [WIP] Migrate to pytest (#6729)
You're right that this is a ton of changes for a project that is not actively developed. It may make more sense to do a hard fork here:
My goal is to get at least 2 more years of stability for PyMC3: I think we gain a lot through a few tasks that are mostly automated
* Drop nose in favor of pytest (nose is long deprecated, pymc devs are more familiar with pytest, pytest has an active ecosystem of tools to help focus bug fixes and new tests)
* Drop Python 2 support (2to3 makes this fairly easy, Python2 is no longer supported in a month, and it lowers the code footprint needed to maintain a library)
* Bump requirements of major libraries and test against current Python/numpy/scipy stack (this involves tracking down warnings and using static linters to keep up to date with deprecations in those libraries)
Do you have an opinion on the idea of a fork? I of course love Theano and appreciate all your work, but it seems as though your goals are to keep a lightly maintained version of Theano for legacy users, while PyMC3 is perhaps more willing to sacrifice backwards compatibility (and current stability! we certainly lack your familiarity with the codebase) for some amount of iteration.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<#6729?email_source=notifications&email_token=AABMF645HOX56NZFZQYURRDQSVYDTA5CNFSM4JKXCFN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDSHQHQ#issuecomment-551843870>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AABMF66G4L5JAR4V24N32TTQSVYDTANCNFSM4JKXCFNQ>.
…-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
|
|
Thanks for the thoughtful reply -- I think a lot of my response boils down to the subtle difference in aims: it sounds like you aim to keep Theano running as-is for users, and I am aiming to keep Theano as a robust platform for PyMC3 to run on top of. It feels as though we are running into more issues - small and large - in PyMC3, and I am trying to figure out how I can hop in and start fixing those here, as well as to make it easier for other PyMC3 contributors to help. Broadly the two large issues I am looking at right now (I know others have issues) are:
At the same time, I am looking towards more deprecations and subtle errors popping up in the future, and I would love to be able to submit fixes for those. I understand there are other (better) ways of fixing these problems (like me getting more familiar with the current tooling and codebase), but given the amount of time I can spend submitting fixes, this is the fastest path I can see. Moving the tests to pytest was partly a way for me to get familiar with the test suite, and a way to remove a dependency that has been deprecated for ~5 years. You are totally right that we do not get any immediate features from the switch -- I'm running coverage.py and profiling with it, but I know there are equivalent nose plugins. This is another change that makes development more comfortable and efficient for me. |
This is to give us access to tools we are more used to using, as well as ones that are actively maintained. There remains a lot of low-hanging fruit for improving the use of pytest:
yieldtestsThis remains a WIP because I need to update the CI scripts.
Somewhat surprisingly, running the test suite requires only ~11minutes on my machine. I imagine it might take longer if some of the skipped tests no longer got skipped.
Also,
np.ceil,np.floor, andnp.truncno longer work with theano tensors.