Added tests for newer versions of ipython and python #44
Conversation
|
hey @mariusvniekerk, thanks for the patch! Do you by any chance have a link to the relevant changes in https://github.com/jupyter/notebook to compare against? |
|
I've essentially disabled that test for now, since the desired behavior is undefined across the set of python versions we support |
|
@ssanderson tests pass now. additionally for older versions of notebook/ipython i had to pin to tornado<5. |
|
@mariusvniekerk I'll take a look at this this evening. |
There was a problem hiding this comment.
@mariusvniekerk I posted a few small comments. Let me know if you have any questions.
| - python: 3.4 | ||
| env: IPYTHON=4 | ||
|
|
||
| - python: 3.6 |
There was a problem hiding this comment.
Does IPython 6 require 3.6 or higher? My inclination would be to test on the oldest supported version of Python, since that's the most likely to surface version compatibility bugs.
There was a problem hiding this comment.
It does not, though it's probably a good idea to test a newer version of python too.
pgcontents/pgmanager.py
Outdated
| klass = PostgresContentsManager | ||
| kw = super(klass, self)._checkpoints_kwargs_default() | ||
| except AttributeError: | ||
| kw = {} |
There was a problem hiding this comment.
It looks like this method changed names from _checkpoints_kwargs_default to _default_checkpoints_kwargs in jupyter/notebook@a91af4d#diff-c604f4531aba170341bbf6e9a155d5daR124, which upgraded to the 4.x series of traitlets. The current defaults are: {'parent': self, 'log': self.log}.
Handling this gracefully across multiple Jupyter versions seems like a pain. I think the right way to handle this is probably to just drop support for the 3.x series of IPython/Jupyter (it's two major versions old now, so I don't feel too bad about losing support for it. We've also had #32 open for a while with no objections raised.) and upgrade to the newer decorator-based traitlets APIs.
Dropping 3.x support is probably more work than we want to do in this PR, so until I get around to ripping out the old-style traitlets code, I think the right thing to do here is probably to hard code the defaults from upstream. Does that seem reasonable to you?
There was a problem hiding this comment.
alternatively i can just try to call the other method
| @@ -163,6 +163,17 @@ def test_list_checkpoints_sorting(self): | |||
| ) | |||
| ) | |||
|
|
|||
| # skip test as it is not satisfyable across supported notebook versions | |||
| def test_delete_non_empty_dir(self): | |||
| # ContentsManager has different behaviour in notebook 5.5+ | |||
There was a problem hiding this comment.
So, from reading the discussion on jupyter/notebook#3108, it seems like the reason notebook now allows deleting non-empty directories is that they switched to using send2trash for deletions by default, which means it's possible for a user to recover a lost directory that they accidentally deleted. Since deletions in pgcontents are still irreversible, my inclination would be to continue to test that we don't support deletion of nonempty directories when using PostgresContentsManager.
I think the easiest way to test that ensure that behavior is to make this method conditional on the value of self.config.NotebookApp.contents_manager_class. If it's a subclass of PostgresContentsManager, we should keep the test for the old behavior. For FileContentsManager we can just call super() to test that we haven't somehow broken the default behavior from upstream. For HybridContentsManager, the expected behavior would depend on how the sub-managers are configured, so the right test would probably just be to assert that we prevent deletions in the subdirectory owned by the postgres manager.
Apologies for the somewhat complex feedback here: this suite is a bit convoluted, but since this is testing the code that controls whether/how we delete user data on Quantopian, I want to err on the side of caution. If you prefer, I can also open a PR against this branch with the changes I have in mind for the suite.
There was a problem hiding this comment.
That's probably for the best
| 'ipy3': ['ipython[test,notebook]<4.0'], | ||
| 'ipy4': ['ipython<6.0', 'notebook[test]>=4.0,<5.0'], | ||
| 'ipy3': ['ipython[test,notebook]<4.0', 'tornado<5.0'], | ||
| 'ipy4': ['ipython<6.0', 'notebook[test]>=4.0,<5.0', 'tornado<5.0'], |
There was a problem hiding this comment.
Why the tornado change? I wouldn't expect pgcontents to care much about the version of tornado.
There was a problem hiding this comment.
pgcontents doesn't care, but notebook very much does and older versions of notebook don't pin tornado to an upper bound. tornado 5 did enough backward incompatible changes to basically make it not work unless you are running a substantially closer to master version of notebook.
setup.py
Outdated
| 'ipy4': ['ipython<6.0', 'notebook[test]>=4.0,<5.0'], | ||
| 'ipy3': ['ipython[test,notebook]<4.0', 'tornado<5.0'], | ||
| 'ipy4': ['ipython<6.0', 'notebook[test]>=4.0,<5.0', 'tornado<5.0'], | ||
| 'ipy6': ['ipython>6.0', 'notebook[test]>=5.0'] |
There was a problem hiding this comment.
Should this be >=? Otherwise I think we're strictly excluding 6.0.
|
hey @mariusvniekerk just wanted to let you know this is still on my radar. Are you working on this for something in particular? I don't want you to be blocked on me looking at this. |
|
I'm not blocked by it, for now i can run from my branch internally. I have another PR coming after this one that adds the ability to copy things between contentsmanagers when using the hybrid contents manager. |
|
hey @mariusvniekerk, just wanted to let you know that @TimShawver and I (but mostly Tim) merged this as part of #46. Thanks for the help, and sorry I took so long to follow through on this. |
There is a small change in behavior for one of the contents manager pieces in notebook 5.5. This adds some support for newer ipython versions and adds some more tests. Resolves #28