Skip to content
This repository has been archived by the owner on Sep 11, 2023. It is now read-only.

[msm] use Ipython's ShimModule approach to warn upon access of a moved module. #805

Merged
merged 14 commits into from
May 18, 2016

Conversation

marscher
Copy link
Member

This adresses #550. Remove those packages again in version 2.3

The user will get a "ShimWarning", when he/she tries to access an attribute from a moved module.
All msmtools sub-packages like analysis and estimation are now ShimModules, which redirects the functionality to the actual implementation, but print a warning upon doing so.

This way we can safely remove pyemma.msm.msmtools stuff in the next minor version.

@franknoe
Copy link
Contributor

Good cal. What does "Shim" mean? Is that a standard term?

Am 17/05/16 um 18:42 schrieb Martin K. Scherer:

This adresses #550 #550.
Remove those packages again in version 2.3

The user will get a "ShimWarning", when he/she tries to access an
attribute from a moved module.
All msmtools sub-packages like analysis and estimation are now
ShimModules, which redirects the functionality to the actual
implementation, but print a warning upon doing so.

This way we can safely remove pyemma.msm.msmtools stuff in the next
minor version.


    You can view, comment on, or merge this pull request online at:

#805

    Commit Summary


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#805


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@franknoe
Copy link
Contributor

what about a DeprecationWarning?

Am 17/05/16 um 18:42 schrieb Martin K. Scherer:

This adresses #550 #550.
Remove those packages again in version 2.3

The user will get a "ShimWarning", when he/she tries to access an
attribute from a moved module.
All msmtools sub-packages like analysis and estimation are now
ShimModules, which redirects the functionality to the actual
implementation, but print a warning upon doing so.

This way we can safely remove pyemma.msm.msmtools stuff in the next
minor version.


    You can view, comment on, or merge this pull request online at:

#805

    Commit Summary


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#805


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@marscher
Copy link
Member Author

marscher commented May 17, 2016 via email

@marscher
Copy link
Member Author

marscher commented May 17, 2016 via email

@marscher
Copy link
Member Author

ShimModules were used during the transition from Ipython3 to Jupyter, where some packages got split up and so the developers decided to warn upon the usage of the old path. So I'd guess at least some users could have seen "ShimWarnings" along their path.

@mpharrigan
Copy link

https://en.wikipedia.org/wiki/Shim_%28spacer%29

Shims are used to "fill in the gaps". The term is stolen from construction.

Thank you guys so much for discussing everything in English! :) Much appreciated

@marscher
Copy link
Member Author

marscher commented May 17, 2016 via email

@franknoe
Copy link
Contributor

Thanks Matthew! We're pretty international in the group, so it wouldn't
even make sense for us to discuss in German. And this way we learn new
words :)

How weird, so we use IPython's deprecation warning also outside IPython
because IPython refuses to show standard deprecation warnings. Anyway,
if the warning message is clear, I don't care which Warning object is
generated. It's just a bit confusing, because normally we would trigger
a DeprecationWarning.

What about providing a shortcut e.g. warn_deprecated in
pyemma.util that triggers the right warning. If Jupyter decides to
change filters in the future, we can go back to normal warnings there.

Am 17/05/16 um 20:09 schrieb Matthew Harrigan:

https://en.wikipedia.org/wiki/Shim_%28spacer%29

Shims are used to "fill in the gaps". The term is stolen from
construction.

Thank you guys so much for discussing everything in English! :) Much
appreciated


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#805 (comment)


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@coveralls
Copy link

coveralls commented May 17, 2016

Coverage Status

Coverage decreased (-0.3%) to 86.012% when pulling 8c1782f on marscher:deprecate_msm_lowlevel_package into f18b417 on markovmodel:devel.

@marscher
Copy link
Member Author

Actually as I have demonstrated, the fault lies not on IPythons side, but has something to do with Anacondas default settings.

Wrapping around this issue in the meantime seems like a good solution.

@marscher
Copy link
Member Author

It is not Anacondas fault, but mine, since I didn't read the Python documentation:
https://docs.python.org/2/library/warnings.html#default-warning-filters

Developer related warnings (eg. DeprecationWarnings) are filtered by default. So we should really think of introducing our own type, since we want the users to be informed if they are using features, which will be removed soon.

@franknoe
Copy link
Contributor

What about changing the filter to show DeprecationWarnings?

Am 17/05/16 um 20:55 schrieb Martin K. Scherer:

It is not Anacondas fault, but mine, since I didn't read the Python
documentation:
https://docs.python.org/2/library/warnings.html#default-warning-filters

Developer related warnings (eg. DeprecationWarnings) are filtered by
default. So we should really think of introducing our own type, since
we want the users to be informed if they are using features, which
will be removed soon.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#805 (comment)


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@marscher
Copy link
Member Author

Am 17.05.2016 um 21:00 schrieb Frank Noe:

What about changing the filter to show DeprecationWarnings?
This is the simplest but also the worst solution, because we would then probably
flood the users output with tons of DeprecationWarnings only intended to be shown to developers.

Since the warnings filter is global, it applies for every imported package. Especially jupyter is very verbose.

So I guess introducing our own type will not hurt anybody. The code needs to changed in two places.

@franknoe
Copy link
Contributor

You're right.

Am 17/05/16 um 21:08 schrieb Martin K. Scherer:

Am 17.05.2016 um 21:00 schrieb Frank Noe:

What about changing the filter to show DeprecationWarnings?
This is the simplest but also the worst solution, because we would
then probably
flood the users output with tons of DeprecationWarnings only intended
to be shown to developers.

Since the warnings filter is global, it applies for every imported
package. Especially jupyter is very verbose.

So I guess introducing our own type will not hurt anybody. The code
needs to changed in two places.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#805 (comment)


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@marscher
Copy link
Member Author

It is very unclear to me why those warnings are note triggered on Travis for Python2. The tests pass for Py2 on my machine...

@marscher
Copy link
Member Author

It seems like Nose is catching them cleverly. I really want to transition to pytest soon...

@franknoe
Copy link
Contributor

Maybe the nose environment changes the filters?

Am 18/05/16 um 00:34 schrieb Martin K. Scherer:

It seems like Nose is catching them cleverly. I really want to
transition to pytest soon...


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#805 (comment)


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@marscher
Copy link
Member Author

I've done everything I could to have a clean filters environment. It looks like how the tests are invoked.. If the whole suite is run, then error occurs, but if run alone it works.

@franknoe
Copy link
Contributor

Hm, no idea.
BTW, do we still need pyemma/util/_ext/shimmodule.py now?

@marscher
Copy link
Member Author

Yes. This is doing all the magic. I've just interchanged the type of warning

@marscher
Copy link
Member Author

I see... nosetest is changing the default import behaviour. So this can not work. The warnings show up as expected in Travis during the first (nose import everything)...

This is actually not the first time I encountered problems with nose - I'm really in the mood making the transition to pytest now.

@franknoe
Copy link
Contributor

now as in now?
Can you change the import behavior of nosetest?

Am 18/05/16 um 01:04 schrieb Martin K. Scherer:

I see... nosetest is changing the default import behaviour. So this
can not work. The warnings show up as expected in Travis during the
first (nose import everything)...

This is actually not the first time I encountered problems with nose -
I'm really in the mood making the transition to pytest now.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#805 (comment)


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@marscher
Copy link
Member Author

It is impossible to change this behaviour according to the documentation. So yes, I want to do it now.

@franknoe
Copy link
Contributor

Sounds like a big change. I guess all tests need to be changed, or is
pytest just a different driver which uses the same Test classes?

Am 18/05/16 um 01:11 schrieb Martin K. Scherer:

It is impossible to change this behaviour according to the
documentation. So yes, I want to do it now.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#805 (comment)


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@marscher
Copy link
Member Author

We're using the standard unit testing framework from Python. This framework provides the Unittest classes to set up test cases. This way we are pretty much independent of the running framework. The running framework simply collects the all the test files (and doctests) and runs them.

For instance nose ignores the tests in pyemma._base package because it starts with an underscore. We even had undiscovered bugs because of that.

The change is actually not that massive and it only requires the adoption of devtools/conda-recipe/run_test.py

@franknoe
Copy link
Contributor

Ah I see! I though you wanted to change the way tests are written, but
it's just the driver. I have no experience with pytest, but sounds good
to me. Have a try.

Am 18/05/16 um 01:17 schrieb Martin K. Scherer:

We're using the standard unit testing framework from Python. This
framework provides the Unittest classes to set up test cases. This way
we are pretty much independent of the running framework. The running
framework simply collects the all the test files (and doctests) and
runs them.

For instance nose ignores the tests in pyemma._base package because it
starts with an underscore. We even had undiscovered bugs because of that.

The change is actually not that massive and it only requires the
adoption of devtools/conda-recipe/run_test.py


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#805 (comment)


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@marscher
Copy link
Member Author

pytest even has a (working) plugin to distribute tests to multiple subprocesses, which the nosetests plugin couldn't achieve every time I tested it.

I will implement the change and then see how it will be working.

@franknoe
Copy link
Contributor

ok, thanks

Am 18/05/16 um 01:20 schrieb Martin K. Scherer:

pytest even has a (working) plugin to distribute tests to multiple
subprocesses, which the nosetests plugin couldn't achieve every time I
tested it.

I will implement the change and then see how it will be working.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#805 (comment)


Prof. Dr. Frank Noe
Head of Computational Molecular Biology group
Freie Universitaet Berlin

Phone: (+49) (0)30 838 75354
Web: research.franknoe.de

Mail: Arnimallee 6, 14195 Berlin, Germany

@marscher
Copy link
Member Author

As it turns out pytest is even more buggy than nose 👍
We can run the new tests without any problem under Python3, so I disabled the run under Python 2 - but still it is really annoying...

@marscher
Copy link
Member Author

pytest-dev/pytest#1235

@coveralls
Copy link

coveralls commented May 18, 2016

Coverage Status

Coverage decreased (-0.2%) to 86.058% when pulling a96ce1d on marscher:deprecate_msm_lowlevel_package into f18b417 on markovmodel:devel.

@marscher marscher force-pushed the deprecate_msm_lowlevel_package branch from a96ce1d to 005532e Compare May 18, 2016 02:21
@coveralls
Copy link

coveralls commented May 18, 2016

Coverage Status

Coverage increased (+0.04%) to 86.472% when pulling 98b47bc on marscher:deprecate_msm_lowlevel_package into c00f8fd on markovmodel:devel.

@coveralls
Copy link

coveralls commented May 18, 2016

Coverage Status

Coverage increased (+0.03%) to 86.465% when pulling 98b47bc on marscher:deprecate_msm_lowlevel_package into c00f8fd on markovmodel:devel.

@marscher marscher merged commit db11507 into markovmodel:devel May 18, 2016
@marscher marscher deleted the deprecate_msm_lowlevel_package branch May 18, 2016 04:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants