Skip to content
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

fix payload keys #2872

Merged
merged 1 commit into from Feb 1, 2013
Merged

fix payload keys #2872

merged 1 commit into from Feb 1, 2013

Conversation

minrk
Copy link
Member

@minrk minrk commented Feb 1, 2013

A few changes left out from PR #2854

prevented pager or set_next_input (%load) from working in the notebook.

A few changes left out from PR ipython#2854

prevented pager or set_next_input (%load) from working in the notebook.
@ellisonbg
Copy link
Member

So I am assuming these keys got updated in the actual kernel side sending?

@minrk
Copy link
Member Author

minrk commented Feb 1, 2013

yes - If I had missed it on both sides, there would be no symptom. Just a matter of find/replace not looking in the javascript files.

@tkf
Copy link
Contributor

tkf commented Feb 1, 2013

Does it mean #2854 is a backward incompatible change? In this case it is not difficult to make client compatible to both versions, but I think strictly speaking protocol major version should be bumped.

I wounder what happens if I connect to old kernel using for example ipython console --existing ... now.

@minrk
Copy link
Member Author

minrk commented Feb 1, 2013

@tkf protocol major version has been bumped. We do not need to rev it ever again between now and 0.14

@tkf
Copy link
Contributor

tkf commented Feb 1, 2013

So, I guess that's the answer to my question in #2792. You don't update major version if there is a backward incompatible change in dev branch. I just wanted to note that you will have problem when you use dev branch and another version (e.g., running ipython kernel using 0.13 and connecting to it by ipython console using dev branch).

@minrk
Copy link
Member Author

minrk commented Feb 1, 2013

So, I guess that's the answer to my question in #2792.

Right, I think I answered there as well. We rev it only the first time we make such a change, which this time around was adding the version itself. Though we need to think about the difference between this and a protocol version (set_next_input and pager are not in the message spec), so I am not sure what we would do if this were a new change, since it does not actually affect the message spec as it is defined now.

I just wanted to note that you will have problem when you use dev branch and another version (e.g., running ipython kernel using 0.13 and connecting to it by ipython console using dev branch).

The wire format isn't the same, it won't even get a chance to miss the payload. IPython dev is not at all backwards compatible with 0.13.

@tkf
Copy link
Contributor

tkf commented Feb 1, 2013

I am sorry I missed your answer in #2792. And you are right that #2854 does not change message spec. The message spec does not specify what can be in payload so we can put virtually anything to it. Yes, it would be nice if there is a way to track these changes (though it would be bad if it slows down IPython development...).

The wire format isn't the same, it won't even get a chance to miss the payload. IPython dev is not at all backwards compatible with 0.13.

I didn't know that. But that's not my point. You can think of the case there is a kernel of dev branch before #2854 in a remote machine and connecting to it by the client of dev branch after #2854. Something like this can happen with 0.14 and 0.15.dev in the future.

@minrk
Copy link
Member Author

minrk commented Feb 1, 2013

It is true that we need to keep track of the payload stuff, but it mostly brings up the point that it makes no sense that the payload is the full Python import path. @ellisonbg do you have a better idea for the payload keys? It makes no sense for non-Python kernels to know precisely where IPython has defined functions that invoke the pager, and it also makes no sense that a change in the IPython module layout would affect frontends, both of which are true right now. Why are these keys not just "pager" and "set_next_input"?

You can think of the case there is a kernel of dev branch before #2854 in a remote machine and connecting to it by the client of dev branch after #2854. Something like this can happen with 0.14 and 0.15.dev in the future.

Correct. IPython 0.14dev was, before #2854, already totally incompatible with IPython 0.13. In the course of 0.15dev, the first time we make a change that frontends need to know about, the protocol spec should be updated. Never again after that and before release. I would consider it a bug that frontends need to know the import path of the pager function, so I would expect this to change one more time before 0.14, which again should have no effect on the protocol version since it is already updated.

@minrk
Copy link
Member Author

minrk commented Feb 1, 2013

Okay, adding this to another msg spec piece that should be revisited, just not here:

Payload's currently track their source, which is the full Python path. This is definitely inappropriate as we consider non-Python kernels, and it is not a good choice for the handler switch in frontends. One option is to add a 'handler' key, that is not dependent on where the Python call originated (e.g. 'pager' or 'set_next_input').

@ellisonbg
Copy link
Member

I think this should be merged as is to fix the immediate breakage in the code. But I also agree that we need to have a future discussion about this part of the message spec. I think these names should probably be logical ('pager' and 'set_next_input'). But I also think that 'source' is a misleading name of the key (maybe handler). But these changes break the message spec further so we need to consider them carefully.

Merging now as this fix needs to get in.

ellisonbg added a commit that referenced this pull request Feb 1, 2013
@ellisonbg ellisonbg merged commit 4af5357 into ipython:master Feb 1, 2013
@minrk minrk deleted the fixpayloads branch February 1, 2013 19:17
tkf added a commit to tkf/emacs-ipython-notebook that referenced this pull request Feb 10, 2013
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants