Add 'unique' option to history_request messaging protocol #2609

Merged
merged 6 commits into from Jan 11, 2013

Conversation

Projects
None yet
4 participants
@tkf
Contributor

tkf commented Nov 20, 2012

I added boolean 'unique' option to history_request messaging protocol, to show only unique history entries. This option is false by default so that the default behavior is not changed. Currently this option has effect only on 'search' request. Magic command %history can take -u option to specify this option.

@takluyver

View changes

IPython/core/tests/test_history.py
@@ -70,35 +73,53 @@ def test_history():
gothist = ip.history_manager.get_range(-1, 1, 4)
nt.assert_equal(list(gothist), zip([1,1,1],[1,2,3], hist))
+ newhist = [(2, i + 1, c) for (i, c) in enumerate(newcmds)]

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Nov 20, 2012

Member

If you call enumerate with start=1, we shouldn't need i+1.

@takluyver

takluyver Nov 20, 2012

Member

If you call enumerate with start=1, we shouldn't need i+1.

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Nov 21, 2012

Contributor

I didn't know that enumerate can take "start" argument!

@tkf

tkf Nov 21, 2012

Contributor

I didn't know that enumerate can take "start" argument!

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Nov 20, 2012

Member

I can see the use of adding an option to the %hist magic, but I wouldn't add it to the messaging protocol without anything that needs it - it's just added complexity.

Member

takluyver commented Nov 20, 2012

I can see the use of adding an option to the %hist magic, but I wouldn't add it to the messaging protocol without anything that needs it - it's just added complexity.

@tkf

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Nov 21, 2012

Contributor

Well, I need it, because Emacs client has interactive history search UI which shows search results as you type ("QuickSilver style"). For such kind of UI speed is critical. That's why it is preferred to drop duplicated entries on kernel side rather than client side.

But I understand that you don't want to extend the protocol in such a way that none of official clients benefit. If you decide not to extend the protocol at this point, I will remove the commit for the protocol.

Contributor

tkf commented Nov 21, 2012

Well, I need it, because Emacs client has interactive history search UI which shows search results as you type ("QuickSilver style"). For such kind of UI speed is critical. That's why it is preferred to drop duplicated entries on kernel side rather than client side.

But I understand that you don't want to extend the protocol in such a way that none of official clients benefit. If you decide not to extend the protocol at this point, I will remove the commit for the protocol.

@Carreau

This comment has been minimized.

Show comment Hide comment
@Carreau

Carreau Nov 22, 2012

Member

Let's do a Protocol Extension Proposal. It is unwise to extend the protocol
strp by step.

I think protocol will need extension at some point in the futur. Then we
will see all the changes that are requested. See the best way to integrate
them.

Then we'll bump the version number, implement what need to be done.

Does this seem like a good plan?
Le 21 nov. 2012 19:54, "Takafumi Arakaki" notifications@github.com a
écrit :

Well, I need it, because Emacs client has interactive history search UI
which shows search results as you type ("QuickSilver style"). For such kind
of UI speed is critical. That's why it is preferred to drop duplicated
entries on kernel side rather than client side.

But I understand that you don't want to extend the protocol in such a way
that none of official clients benefit. If you decide not to extend the
protocol at this point, I will remove the commit for the protocol.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2609#issuecomment-10609613.

Member

Carreau commented Nov 22, 2012

Let's do a Protocol Extension Proposal. It is unwise to extend the protocol
strp by step.

I think protocol will need extension at some point in the futur. Then we
will see all the changes that are requested. See the best way to integrate
them.

Then we'll bump the version number, implement what need to be done.

Does this seem like a good plan?
Le 21 nov. 2012 19:54, "Takafumi Arakaki" notifications@github.com a
écrit :

Well, I need it, because Emacs client has interactive history search UI
which shows search results as you type ("QuickSilver style"). For such kind
of UI speed is critical. That's why it is preferred to drop duplicated
entries on kernel side rather than client side.

But I understand that you don't want to extend the protocol in such a way
that none of official clients benefit. If you decide not to extend the
protocol at this point, I will remove the commit for the protocol.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2609#issuecomment-10609613.

@tkf

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Nov 22, 2012

Contributor

Sorry, could you elaborate more? I understand that it is better to have IPEP before extending messaging protocol. But the rest of the part is unclear.

I think protocol will need extension at some point in the futur. Then we
will see all the changes that are requested. See the best way to integrate
them.

Do you mean we need IPEP for (1) protocol extension system, (2) unique option in history_request message or (3) possible history_request message options? I guess (1) is too generic for non-dev person like me to write (well, I can try, though). I feel (2) is too specific for an IPEP, comparing to the existing ones. For (2), I guess discussion in this PR will do the job. I can start (3), but I have no other request options I can think of at this moment.

Also, for history search request, I don't think we need protocol extension system (1), because if a specific client needs a specific history request mechanism, that client can load its own Python module in the kernel and then send the request back to the client via JSON repr. But that means IPython should provide core.history.HistoryAccessor as a public API, or the client ends up relying on internal implementation of IPython. I think providing full-access to core.history.HistoryAccessor options via messaging protocol is much cleaner than making core.history.HistoryAccessor public. I am not saying that protocol extension system is unnecessary. I just don't think history search request protocol require the extension system.

Then we'll bump the version number, implement what need to be done.

What version number? IPython? Or are we going to have a protocol version number?

Contributor

tkf commented Nov 22, 2012

Sorry, could you elaborate more? I understand that it is better to have IPEP before extending messaging protocol. But the rest of the part is unclear.

I think protocol will need extension at some point in the futur. Then we
will see all the changes that are requested. See the best way to integrate
them.

Do you mean we need IPEP for (1) protocol extension system, (2) unique option in history_request message or (3) possible history_request message options? I guess (1) is too generic for non-dev person like me to write (well, I can try, though). I feel (2) is too specific for an IPEP, comparing to the existing ones. For (2), I guess discussion in this PR will do the job. I can start (3), but I have no other request options I can think of at this moment.

Also, for history search request, I don't think we need protocol extension system (1), because if a specific client needs a specific history request mechanism, that client can load its own Python module in the kernel and then send the request back to the client via JSON repr. But that means IPython should provide core.history.HistoryAccessor as a public API, or the client ends up relying on internal implementation of IPython. I think providing full-access to core.history.HistoryAccessor options via messaging protocol is much cleaner than making core.history.HistoryAccessor public. I am not saying that protocol extension system is unnecessary. I just don't think history search request protocol require the extension system.

Then we'll bump the version number, implement what need to be done.

What version number? IPython? Or are we going to have a protocol version number?

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Nov 22, 2012

Member

I think @Carreau is talking about a versioned protocol, and suggesting that we have an IPEP for 'kernel messaging protocol 1.1' (or 2, or whatever it gets called). By rough analogy, [Python] PEP 3154 is for 'pickle protocol version 4'.

I can see the sense behind that - a third party client like EIN will clearly need to know whether the kernel supports the fields it wants to use. But I think we should start with a thread on the ipython-dev mailing list about how we should evolve the messaging protocol.

Member

takluyver commented Nov 22, 2012

I think @Carreau is talking about a versioned protocol, and suggesting that we have an IPEP for 'kernel messaging protocol 1.1' (or 2, or whatever it gets called). By rough analogy, [Python] PEP 3154 is for 'pickle protocol version 4'.

I can see the sense behind that - a third party client like EIN will clearly need to know whether the kernel supports the fields it wants to use. But I think we should start with a thread on the ipython-dev mailing list about how we should evolve the messaging protocol.

@tkf

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Nov 22, 2012

Contributor

I see. I will post some idea to the ML later. But please start the discussion if you want.

Contributor

tkf commented Nov 22, 2012

I see. I will post some idea to the ML later. But please start the discussion if you want.

@tkf

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Nov 22, 2012

Contributor

Probably it's better to remove the commit for messaging protocol from this PR, so that you can merge this PR? The commit for messaging protocol is completely independent.

Contributor

tkf commented Nov 22, 2012

Probably it's better to remove the commit for messaging protocol from this PR, so that you can merge this PR? The commit for messaging protocol is completely independent.

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Nov 22, 2012

Member

That sounds like a plan - we can review the simple change to the %hist
magic, and discuss the messaging protocol separately.

On 22 November 2012 14:15, Takafumi Arakaki notifications@github.comwrote:

Probably it's better to remove the commit for messaging protocol from this
PR, so that you can merge this PR? The commit for messaging protocol is
completely independent.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2609#issuecomment-10635637.

Member

takluyver commented Nov 22, 2012

That sounds like a plan - we can review the simple change to the %hist
magic, and discuss the messaging protocol separately.

On 22 November 2012 14:15, Takafumi Arakaki notifications@github.comwrote:

Probably it's better to remove the commit for messaging protocol from this
PR, so that you can merge this PR? The commit for messaging protocol is
completely independent.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2609#issuecomment-10635637.

@tkf

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Nov 22, 2012

Contributor

I removed the commits related to the messaging protocol (b9f1b2e65ae5dbd4e7b4c77e0f95c4ddc0539b51 and a2f98b734a8930b4ffd14f9db63758a9be363833) and applied the suggestion by @takluyver.

Contributor

tkf commented Nov 22, 2012

I removed the commits related to the messaging protocol (b9f1b2e65ae5dbd4e7b4c77e0f95c4ddc0539b51 and a2f98b734a8930b4ffd14f9db63758a9be363833) and applied the suggestion by @takluyver.

+ @argument(
+ '-u', dest='unique', action='store_true',
+ help="""
+ when searching history using `-g`, show only unique history.

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Nov 22, 2012

Member

More specifically, how does this work with the numbering? Will it show the line number of the first identical entry, the last one, or is it arbitrary?

@takluyver

takluyver Nov 22, 2012

Member

More specifically, how does this work with the numbering? Will it show the line number of the first identical entry, the last one, or is it arbitrary?

This comment has been minimized.

Show comment Hide comment
@Carreau

Carreau Nov 22, 2012

Member

Don't now sql well, but looking for the answer to this question, DISTINCTseem a more appropriate way than GROUP BY.
Am I missing something ?

@Carreau

Carreau Nov 22, 2012

Member

Don't now sql well, but looking for the answer to this question, DISTINCTseem a more appropriate way than GROUP BY.
Am I missing something ?

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Nov 22, 2012

Contributor

I tried DISTINCT first, but it turned out that it compares whole row (i.e., including session and line). I am not SQL expert and there is many option for constructing SQL code so I am not 100% sure, but I think it picks up the latest entry. I think this choice makes sense. We can have more precise control by doing SELECT MAX(session) MAX(line) ... or SELECT MIN(session) MIN(line) ... but I think it over complicates the history class. You will need to add MAX/MIN in _run_sql method.

@tkf

tkf Nov 22, 2012

Contributor

I tried DISTINCT first, but it turned out that it compares whole row (i.e., including session and line). I am not SQL expert and there is many option for constructing SQL code so I am not 100% sure, but I think it picks up the latest entry. I think this choice makes sense. We can have more precise control by doing SELECT MAX(session) MAX(line) ... or SELECT MIN(session) MIN(line) ... but I think it over complicates the history class. You will need to add MAX/MIN in _run_sql method.

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Dec 12, 2012

Contributor

So, it looks like we need MAX or MIN if we want predictable numbers for session and line.

If the SELECT statement is an aggregate query with a GROUP BY clause, then each of the expressions specified as part of the GROUP BY clause is evaluated for each row of the dataset.
[...]
Each expression in the result-set is then evaluated once for each group of rows. If the expression is an aggregate expression, it is evaluated across all rows in the group. Otherwise, it is evaluated against a single arbitrarily chosen row from within the group. If there is more than one non-aggregate expression in the result-set, then all such expressions are evaluated for the same row.

-- SQLite Query Language: SELECT

@tkf

tkf Dec 12, 2012

Contributor

So, it looks like we need MAX or MIN if we want predictable numbers for session and line.

If the SELECT statement is an aggregate query with a GROUP BY clause, then each of the expressions specified as part of the GROUP BY clause is evaluated for each row of the dataset.
[...]
Each expression in the result-set is then evaluated once for each group of rows. If the expression is an aggregate expression, it is evaluated across all rows in the group. Otherwise, it is evaluated against a single arbitrarily chosen row from within the group. If there is more than one non-aggregate expression in the result-set, then all such expressions are evaluated for the same row.

-- SQLite Query Language: SELECT

@tkf

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Jan 10, 2013

Contributor

Isn't it good to go?

Contributor

tkf commented Jan 10, 2013

Isn't it good to go?

@ellisonbg

This comment has been minimized.

Show comment Hide comment
@ellisonbg

ellisonbg Jan 11, 2013

Member

I will let @takluyver decide about this one.

Member

ellisonbg commented Jan 11, 2013

I will let @takluyver decide about this one.

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Jan 11, 2013

Member

I think it's OK. Thanks, @tkf, I'll merge this now.

Member

takluyver commented Jan 11, 2013

I think it's OK. Thanks, @tkf, I'll merge this now.

takluyver added a commit that referenced this pull request Jan 11, 2013

Merge pull request #2609 from tkf/history-unique
Add 'unique' option to history_request messaging protocol

@takluyver takluyver merged commit 751e347 into ipython:master Jan 11, 2013

1 check passed

default The Travis build passed
Details
@tkf

This comment has been minimized.

Show comment Hide comment
@tkf

tkf Jan 11, 2013

Contributor

Thanks :)

Contributor

tkf commented Jan 11, 2013

Thanks :)

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #2609 from tkf/history-unique
Add 'unique' option to history_request messaging protocol
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment