Opened 5 months ago

Closed 3 weeks ago

#31951 closed defect (fixed)

Fix `..` ellipsis interference with REPL multiline input

Reported by: slelievre Owned by:
Priority: major Milestone: sage-9.5
Component: user interface Keywords: ellipsis, multiline, repl
Cc: cremona, gh-jcamp0x2a, gh-kliem, slelievre Merged in:
Authors: Jonathan Kliem, Kwankyu Lee Reviewers: Kwankyu Lee, Jonathan Kliem
Report Upstream: N/A Work issues:
Branch: 041aeaa (Commits, GitHub, GitLab) Commit: 041aeaadca1846bf8cda58da7f2e3c35267ab23c
Dependencies: Stopgaps:

Status badges

Description

Using ellipsis syntax [a .. b] or (a .. b) interferes with multiline block continuation in Sage REPL input.

For reference, after entering the first two lines below and hitting enter, the prompt is extended with an (auto-indented) third line, as expected:

sage: for n in range(4):
....:     m = 2*n
....:

and the input is evaluated only once the user either manually unindents (e.g. with ctrl-A ctrl-K) and hits enter, or hits enter twice.

By contrast, in all cases below, hitting enter at the end of the second line evaluates the code typed so far, preventing to type a third line:

sage: for n in (1 .. 3):
....:     m = 2*n
sage: for n in [0 .. 3]:
....:     m = 2*n
sage: for n in range(4):
....:     m = (2*n .. 3*n)
sage: for n in range(4):
....:     m = [2*n .. 3*n]

From an initial report by John Cremona on sage-support:

Change History (81)

comment:1 Changed 2 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:2 Changed 6 weeks ago by gh-kliem

Possibly related: <> creates the same problem, as in

sage: foo(L):
....:     K.<a> = L
sage

See #32523.

comment:3 Changed 6 weeks ago by gh-kliem

I guess all of our preparse customizations that are a syntax error in normal python have the same problem:

sage: implicit_multiplication(True)
sage: def foo():
....:     b = 2a

comment:4 Changed 6 weeks ago by gh-kliem

Further problems, not really serious, but it shows problems that one can run into:

sage: a = """
....: sage: Sagemath.
....: """
sage: a
'\nSagemath.\n'

The problem is that one needs to cleanup the syntax in input_transformers_cleanup instead of input_transformers_post.

comment:5 Changed 6 weeks ago by gh-kliem

  • Authors set to Jonathan Kliem
  • Branch set to public/31951
  • Commit set to d9c4e95f8db6ad82291717e822ace7cd68971e51
  • Status changed from new to needs_review

New commits:

d9c4e95fix completeness check for sage special syntax

comment:6 Changed 6 weeks ago by git

  • Commit changed from d9c4e95f8db6ad82291717e822ace7cd68971e51 to 8a4309305d5294d487fc0ad993aac8243f6bc1d5

Branch pushed to git repo; I updated commit sha1. New commits:

8a43093A few cosmetic fixes

comment:7 Changed 6 weeks ago by klee

I made a few stylistic fixes. Feel free to revert any part if you think irrelevant.

The patch works well, looks good. One comment on the syntax_only argument: It is not clear what *Sage's special syntax* the argument is for. What is the criterion? Isn't this R.0 -> R.gen(0) Sage's special syntax?

comment:8 follow-up: Changed 6 weeks ago by gh-kliem

R.0 is fixed as well.

There is correct python syntax. Then ipython has some special syntax, like %time. Then sage has some special syntax.

I would say that special syntax is anything, that gives a SyntaxError without modification. So replacing 2 by Integer(2) is not special syntax. Replacing 2^3 by 2**3 is also not special syntax (and we need really to avoid doing it twice).

I don't know what is a better name for it, but this is really what the bug boils down to:

IPython.input_transformer_manager.check_complete checks the syntax as well, which means it must have access to all our special syntax rules.

comment:9 in reply to: ↑ 8 Changed 6 weeks ago by klee

Replying to gh-kliem:

I don't know what is a better name for it, but this is really what the bug boils down to:

IPython.input_transformer_manager.check_complete checks the syntax as well, which means it must have access to all our special syntax rules.

As I understands it, your code does (added) Sage syntax preparsing down in "token level"(input_transformer_manager level) keeping out problematic transforms indicated by syntax_only, and then does Sage syntax preparing second time after "token level". Am I right?

Then wouldn't it be more safe and efficient to do the transforms (ellipsis and generators) that are necessary to kill the bug in the token level and leave the other transforms after that?

comment:10 Changed 6 weeks ago by gh-kliem

You are right. It would be better to cleanly seperate those two steps.

However, I can't figure it out. It's such a mess...

comment:11 follow-up: Changed 6 weeks ago by gh-kliem

We could also clean this all up and do it more ipython style instead of string replacment.

This would simplify a lot of things. E.g. the tokens treat multiline strings as one line, without further interaction. They also treat implicit and explicit line continuation almost the same.

In this case I would try to replace the preparse function in the interactive shell and then as a last step replace preparse by the appropriate steps in ipython, so that loading a file *.sage is really the same thing as parsing it all in sage.

comment:12 Changed 5 weeks ago by git

  • Commit changed from 8a4309305d5294d487fc0ad993aac8243f6bc1d5 to 99af688e4125f4c0bf0a826ec70ffb6fe6f62b28

Branch pushed to git repo; I updated commit sha1. New commits:

c0c0819document one more fix
99af688fix doc build

comment:13 Changed 5 weeks ago by gh-kliem

  • Status changed from needs_review to needs_work
  • Work issues set to close #4545?

I'm reworking the preparses to fix this.

comment:14 Changed 5 weeks ago by gh-kliem

  • Branch changed from public/31951 to public/31951-new
  • Commit changed from 99af688e4125f4c0bf0a826ec70ffb6fe6f62b28 to 9e7c480c43ef491f92d90c4eb2775f98b6045ba8
  • Status changed from needs_work to needs_review
  • Work issues changed from close #4545? to close #4545 and #30953

The major changes are the following:

  • Instead of worrying about quote_state, we require that all the lines are given at once into preparse_multiple_lines.
  • We add preparse_multiple_lines to cleanup_transformers.
  • We remove preparse from input_transformers_post.
  • The following are now handled via token transformers instead:
    • Backslash division B \ C.
    • Generator construction K.<a> = QuadraticField(2).
    • Calculus like function assigment f(x) = (x + 1).

We add a couple of deprecations.


New commits:

9e7c480rework Sages input preparse, such that the syntax is valid after applying cleanup and token transformers

comment:15 Changed 5 weeks ago by git

  • Commit changed from 9e7c480c43ef491f92d90c4eb2775f98b6045ba8 to db167ddc3c35b6ddbb61f97c7233c2f6d18d005e

Branch pushed to git repo; I updated commit sha1. New commits:

db167ddcorrect ticket number

comment:16 Changed 5 weeks ago by gh-kliem

Replying to klee:

Replying to gh-kliem:

I don't know what is a better name for it, but this is really what the bug boils down to:

IPython.input_transformer_manager.check_complete checks the syntax as well, which means it must have access to all our special syntax rules.

As I understands it, your code does (added) Sage syntax preparsing down in "token level"(input_transformer_manager level) keeping out problematic transforms indicated by syntax_only, and then does Sage syntax preparing second time after "token level". Am I right?

Then wouldn't it be more safe and efficient to do the transforms (ellipsis and generators) that are necessary to kill the bug in the token level and leave the other transforms after that?

Thing is that almost all our changes affect the syntax and need to be done before input_transformers_post. We could do all of them just in cleanup_transformers, but that makes it difficult resolving things like #30953.

comment:17 Changed 4 weeks ago by klee

  • Branch changed from public/31951-new to public/31951
  • Commit changed from db167ddc3c35b6ddbb61f97c7233c2f6d18d005e to 5a7b172568acad1f2ea868608139a946df73a65e

New commits:

9a5362aStylistic fixes
b465513Transfer doctests
3d86554Revert deprecations
fe36d2eMerge branch 'develop' into trac31951
5a7b172Remove unnecessary preparse_ipython

comment:18 follow-up: Changed 4 weeks ago by klee

I made some commits based on your latest work.

My main concern with your work was that it introduced some nontrivial regressions regarding .sage files (not working time and load/attach in file, for instance) by deprecating old preparse stuffs. My commits recover regressions.

It works well for me. I am positive on your work. But I think the patch still needs extensive tests.

comment:19 Changed 4 weeks ago by klee

  • Reviewers set to Kwankyu Lee

comment:20 Changed 4 weeks ago by git

  • Commit changed from 5a7b172568acad1f2ea868608139a946df73a65e to bc8187324de61ff24581d665d4254214133d96f2

Branch pushed to git repo; I updated commit sha1. New commits:

bc81873Fix a doctest failure

comment:21 follow-up: Changed 4 weeks ago by klee

Question:

You deprecated InterfaceShellTransformer and interface_shell_embed. I don't understand these stuffs well, and have concerns whether deprecating them is safe. Would you explain?

comment:22 in reply to: ↑ 21 ; follow-up: Changed 4 weeks ago by gh-kliem

Replying to klee:

Question:

You deprecated InterfaceShellTransformer and interface_shell_embed. I don't understand these stuffs well, and have concerns whether deprecating them is safe. Would you explain?

I have no idea what this is used for or who uses it. It should definitely undergo questions in sage-devel, whether this can be deprecated or whether this needs to be adapted.

comment:23 in reply to: ↑ 18 ; follow-ups: Changed 4 weeks ago by gh-kliem

Replying to klee:

I made some commits based on your latest work.

My main concern with your work was that it introduced some nontrivial regressions regarding .sage files (not working time and load/attach in file, for instance) by deprecating old preparse stuffs. My commits recover regressions.

It works well for me. I am positive on your work. But I think the patch still needs extensive tests.

Instead of doing the token now, we could also leave them for #30953. This should make this ticket here easier.

time is working in .sage. What isn't working anymore is automagic. But automagic isn't meant for multiline things and thus is really a strange concept for files. Line- and cell-magic should be working just fine. (Cell-magic is also a bit strange, as it cannot be exited.) Line-magic wasn't working before in .sage-files.

But yes, this is a regression.

comment:24 in reply to: ↑ 22 Changed 4 weeks ago by klee

Replying to gh-kliem:

Replying to klee:

Question:

You deprecated InterfaceShellTransformer and interface_shell_embed. I don't understand these stuffs well, and have concerns whether deprecating them is safe. Would you explain?

I have no idea what this is used for or who uses it.

If so and if they do not do any harm, I think it is safe to leave them intact.

comment:25 in reply to: ↑ 23 Changed 4 weeks ago by klee

Replying to gh-kliem:

time is working in .sage.

Yes, now. It stopped working with your patch.

What isn't working anymore is automagic. But automagic isn't meant for multiline things and thus is really a strange concept for files. Line- and cell-magic should be working just fine. (Cell-magic is also a bit strange, as it cannot be exited.) Line-magic wasn't working before in .sage-files.

I think any of those magic things is not for .sage files. Even though any one works by accident, we don't need to support it.

comment:26 in reply to: ↑ 23 Changed 4 weeks ago by klee

Replying to gh-kliem:

Instead of doing the token now, we could also leave them for #30953. This should make this ticket here easier.

#30953 is fixed by the patch here both on command line and in .sage file. We can just close #30953 after the present ticket.

comment:27 in reply to: ↑ 23 Changed 4 weeks ago by klee

Replying to gh-kliem:

My main concern with your work was that it introduced some nontrivial regressions regarding .sage files (not working time and load/attach in file, for instance) by deprecating old preparse stuffs. My commits recover regressions.

time and load/attach are Sage things for .sage files. They are not ipython magics.

comment:28 in reply to: ↑ 11 Changed 4 weeks ago by klee

Replying to gh-kliem:

... so that loading a file *.sage is really the same thing as parsing it all in sage.

We should not pursue this. Loading a .sage file needs not be the same thing as typing them in Sage command line. Indeed, code for loading a .sage file does some clever things to speed up running the code in the file, which are not done on command line.

comment:29 Changed 4 weeks ago by gh-kliem

  • Branch changed from public/31951 to public/31951-new
  • Commit changed from bc8187324de61ff24581d665d4254214133d96f2 to db167ddc3c35b6ddbb61f97c7233c2f6d18d005e

comment:30 follow-up: Changed 4 weeks ago by gh-kliem

Sorry for taking your time. I didn't realize until now, that I didn't change the branch yet to the new approach. Hence my comments didn't make a lot of sense to you.

comment:31 Changed 4 weeks ago by git

  • Commit changed from db167ddc3c35b6ddbb61f97c7233c2f6d18d005e to d372575c4254b46d8047a87f371200a516dbf2f3

Branch pushed to git repo; I updated commit sha1. New commits:

d372575do not deprecate InterfaceShellTransformer

comment:32 Changed 4 weeks ago by git

  • Commit changed from d372575c4254b46d8047a87f371200a516dbf2f3 to 5fb96b04218e8eff9c0ea37ab19c4bf92a7994c9

Branch pushed to git repo; I updated commit sha1. New commits:

5fb96b0make doctest stable

comment:33 follow-up: Changed 4 weeks ago by gh-kliem

Fixing #30953 seperate from this ticket, will not work without extra work, as the following doctest already assumes that ipythons preparse detected the implicit line continuation:

        sage: preparse("a \\ b \\") # There is really only one backslash here, it is just being escaped.
        'a  * BackslashOperator() * b \\'
        sage: preparse('''
        ....: f(a,
        ....:   b,
        ....:   c,
        ....:   d) = a + b*2 + c*3 + d*4
        ....: ''')
        '__tmp__ = var("a,b,c,d"); __tmpf__ = a + b*Integer(2) + c*Integer(3) + d*Integer(4); f = symbolic_expression(__tmpf__).function(a,b,c,d)\n'

So, I would argue that it makes sense to introduce the following classes during this ticket:

  • SageBackslashTransformer,
  • SageGenConstructionTransformer,
  • SageCalculusTransformer.

comment:34 Changed 4 weeks ago by gh-kliem

Automagic could be fixed by a replacement as follows:

Replace time -2 by sage_eval("%time -2", globals()) if 'time' not in globals() else sage_eval("time -2", globals())

Does this sound plausible? Do this for every magic keyword.

Last edited 4 weeks ago by gh-kliem (previous) (diff)

comment:35 Changed 4 weeks ago by gh-kliem

  • Branch changed from public/31951-new to public/31951-new2
  • Commit changed from 5fb96b04218e8eff9c0ea37ab19c4bf92a7994c9 to 126ef4f04fef813f41d0536c94c18215419c1588

Sorry about the mess.

This is a much simpler fix. It just creates a second input manager that does Sage's preparsing to check correctness of Sage's syntax. It does not behave as desired with magics, e.g.

sage: ip = get_ipython()
sage: ip._check_complete_transformer.transform_cell("%time 8^^1")
"get_ipython().run_line_magic('time', 'Integer(8)^Integer(1)')\n"

which means it would preparse things after %time twice, but it's sole purpose is to check the correctness of syntax, for which it works just fine.

With the new approach I would like the following follow ups:

  • close #4545 with a doctest
  • have preparse and preparse_file call ip.transform_cell, which means the code will behave exactly as if parsed into the interactive shell (move the code from preparse to a helper function then)
  • #30953: do generator construction and calculus like assignment with token transformation

New commits:

126ef4fvery simple fix for 31951

comment:36 in reply to: ↑ 30 Changed 4 weeks ago by klee

Replying to gh-kliem:

Sorry for taking your time. I didn't realize until now, that I didn't change the branch yet to the new approach. Hence my comments didn't make a lot of sense to you.

What is a mess seems our miscommunication :-)

Yes, you did push your new approach! It fixed a lot of things, and I was happy with that.

comment:37 in reply to: ↑ 33 Changed 4 weeks ago by klee

Replying to gh-kliem:

Fixing #30953 seperate from this ticket, will not work without extra work, as the following doctest already assumes that ipythons preparse detected the implicit line continuation:

        sage: preparse("a \\ b \\") # There is really only one backslash here, it is just being escaped.
        'a  * BackslashOperator() * b \\'
        sage: preparse('''
        ....: f(a,
        ....:   b,
        ....:   c,
        ....:   d) = a + b*2 + c*3 + d*4
        ....: ''')
        '__tmp__ = var("a,b,c,d"); __tmpf__ = a + b*Integer(2) + c*Integer(3) + d*Integer(4); f = symbolic_expression(__tmpf__).function(a,b,c,d)\n'

So, I would argue that it makes sense to introduce the following classes during this ticket:

  • SageBackslashTransformer,
  • SageGenConstructionTransformer,
  • SageCalculusTransformer.

You did introduce these classes in the branch of new approach. Your branch fixed the things above. Then I started working on your branch (with those new classes) because there were regressions in loading .sage files.

comment:38 Changed 4 weeks ago by klee

Then let's straight out our communication first.

(1) You pushed public/31951-new with the new approach. (2) Then I worked on your branch to fix regressions in loading .sage files, and then pushed my branch to public/31951. I think this is a mistake. I should have named it public/31951-new-new or something.

So now the branch (`public/31951-new2) uploaded to this ticket wiped out both of our works!

Could I upload my branch again with name public/31951-new3? Then we can start discussing with the magics, in which there is a miscommunication as well...

Last edited 4 weeks ago by klee (previous) (diff)

comment:39 Changed 4 weeks ago by klee

My branch refactored your new approach and fixed every bugs and regressions (as far as I am concerned) both on command line and in .sage files.

comment:40 Changed 4 weeks ago by gh-kliem

Changing the branch, does not wipe out the work. Indeed I got confused with your branch name. I only saw you last commit and thought I never changed the branch name.

comment:41 Changed 4 weeks ago by gh-kliem

Your branch public/31951 is still intact.

comment:42 follow-up: Changed 4 weeks ago by gh-kliem

It all depends on what we want. The branch public/31951-new2 is a minimalistic fix to the reported bug. As far as I see, it does not have regressions. So I would propose closing the current ticket with this fix and provide a quick fix to users.

We can move the previous work to a new ticket (or new tickets). However, I realized that our approach does not work well with automagic, e.g.

time 8^1 and time 8^^1 should be equivalent to %time 8^1 resp. %time 8^^1, but I did not accomplish this. Did you?

There are a number of phases that the transformation undergoes:

  1. get_ipython().input_transformer_manager.cleanup_transforms: Things like cleaning the prompt as I suggested.
  2. get_ipython().input_transformer_manager.token_transformers: Things like IPythons line magic.
  3. get_ipython().prefilter_manager: Things like IPythons automagic.
  4. get_ipython().input_transformers_post: Currently Sage's preparse.
  5. ip.ast_transformers (Optional additional changes).

Anything that you enter undergoes all 5 steps. However, when checking whether multi-line input is complete, only the first two steps are checked. It makes sense to execute our preparser at step 4, so it works nicely with automagic. It should be executed at step 2 or later to work nicely with (non-auto) magic.

My latest solution to this ticket is to leave Sage's preparser at step 4, but overwrite check_complete with the method from a different input manager that applies Sage's preparser at step 1. Things will be modified before checking whether this is magic, but this should not bother us.

I quickly scanned your fixes and I think they make sense. When executing a file, it should behave exactly as when typed into the shell, so this could be done via ipython_preparse or similar (which calls get_ipython().transform_cell() (steps 1 and 2) and not get_ipython().input_transformer_manager().transform() (steps 1 to 4).

If you agree, you could put your branch on #30953 and we leave the minimalistic fix here. Because of the problems with automagic, I think sage's preparsing should stay at step 4 however (except for checking completeness).

comment:43 in reply to: ↑ 42 ; follow-up: Changed 4 weeks ago by klee

Replying to gh-kliem:

Thanks for the detailed explanation. I only guessed the process.

It all depends on what we want. The branch public/31951-new2 is a minimalistic fix to the reported bug. As far as I see, it does not have regressions. So I would propose closing the current ticket with this fix and provide a quick fix to users.

Anyway we are too late to provide a "quick fix".

We can move the previous work to a new ticket (or new tickets). However, I realized that our approach does not work well with automagic, e.g.

time 8^1 and time 8^^1 should be equivalent to %time 8^1 resp. %time 8^^1, but I did not accomplish this. Did you?

No. I didn't know that. There is no other problem with our approach except this one.

I investigated this. The reason is that ipython applies cleanup_transforms several times to the input string for automagic. Hence time 8^^1 becomes time 8^1 and again time 8**1.

So the solution is to make this code

    # Use ^ for exponentiation and ^^ for xor
    # (A side effect is that **** becomes xor as well.)
    L = L.replace('^', '**').replace('****', '^')  

also to a token transformer(named perhaps SageCaretTransformer or SageExponentiationTransformer?).

With another token transformer, our approach(my branch containing your work) fixes all problems.

If you agree, you could put your branch on #30953 and we leave the minimalistic fix here. Because of the problems with automagic, I think sage's preparsing should stay at step 4 however (except for checking completeness).

As I said above, we can provide one branch solving this ticket and #30953 (and #4545?). All these tickets are just different manifestations of a single problem. Then why separate the solution?

comment:44 in reply to: ↑ 43 Changed 4 weeks ago by klee

So the solution is to make this code

    # Use ^ for exponentiation and ^^ for xor
    # (A side effect is that **** becomes xor as well.)
    L = L.replace('^', '**').replace('****', '^')  

also to a token transformer(named perhaps SageCaretTransformer or SageExponentiationTransformer?).

Or we can replace the code with another *idempotent* code with some regex juggling.

I can try to code the necessary token transformer, but I think you can do it quicker than me.

comment:45 follow-up: Changed 4 weeks ago by gh-kliem

No, just making it idempotent is not enough.

sage: runfile /tmp/2.sage
Traceback (most recent call last)
...
OSError: did not find file '/tmp/Integer(2).sage' to load or attach

Instead of trying to find a workaround for all these problems, we could just leave our preparsing at step 4 and just modify check_complete. Even the backslash transformation is probably best left for step 4.

Now there is a different idea I just had: The current ticket works well with everything except for automagic, which only applies for single-line inputs. In contrast, all our problems only apply for multi-line input. So we could do our replacements at step 1 and 2 for multi-line input and leave it for step 4 for single-line input. This should also work.

comment:46 in reply to: ↑ 45 Changed 4 weeks ago by klee

Replying to gh-kliem:

No, just making it idempotent is not enough.

sage: runfile /tmp/2.sage
Traceback (most recent call last)
...
OSError: did not find file '/tmp/Integer(2).sage' to load or attach

Instead of trying to find a workaround for all these problems, we could just leave our preparsing at step 4 and just modify check_complete. Even the backslash transformation is probably best left for step 4.

Okay. Now I am persuaded.

Now there is a different idea I just had: The current ticket works well with everything except for automagic, which only applies for single-line inputs. In contrast, all our problems only apply for multi-line input. So we could do our replacements at step 1 and 2 for multi-line input and leave it for step 4 for single-line input. This should also work.

Your current patch works really well. What about automagic is not working? Your previous example time 8^^1 works well.

comment:47 Changed 4 weeks ago by klee

Ah. Your current patch does not fix #30953...

comment:48 Changed 4 weeks ago by gh-kliem

sage: %time 8^^1
CPU times: user 17 µs, sys: 2 µs, total: 19 µs
Wall time: 25.3 µs
9
sage: time 8^^1
CPU times: user 33 µs, sys: 3 µs, total: 36 µs
Wall time: 49.8 µs
8
sage: 8^^1
9

Not working. The problem is that 8^^1 is preparsed twice. Automagic is hard to detect and depends in fact on whether a variable of a specific name is defined.

At the moment I really see two ways of doing it:

  • Have an artificial input manager for check_complete.
  • Do the preparsing in steps 1 and 2 except for single-lines, when we keep doing the preparsing at step 4.

comment:49 Changed 4 weeks ago by klee

By combining your current branch (that use artificial input manager for check_complete) with my branch, I think I managed to fix automagic problem. The code is still a mess with lots of doctest errors, but all of them seem fixable.

A question: It seems that ip.input_transformer_manager.transform_cell(...) does not apply input_transformers_post. Am I right? Because of this, now I have lots of doctest errors like

File "src/sage/repl/interpreter.py", line 635, in sage.repl.interpreter.SageBackslashTransformer
Failed example:
    ip.input_transformer_manager.transform_cell(r'''
    a = (2, 3, \
         4)''')
Expected:
    'a = (Integer(2), Integer(3), \\\n     Integer(4))\n'
Got:
    'a = (2, 3, \\\n     4)\n'

Otherwise all seems work well. Let me have some time to tidy up the code.

comment:50 Changed 4 weeks ago by klee

I am ready to upload the combined branch, which works well for all problems which we discussed.

I think it is okay to upload the branch here as it combines the current branch.

comment:51 Changed 4 weeks ago by klee

  • Branch changed from public/31951-new2 to public/31951
  • Commit changed from 126ef4f04fef813f41d0536c94c18215419c1588 to 6456a2af2ba559f2c9766abd8cdcb94eb3597f70

The branch name is again "public/31951".


New commits:

6456a2aRework Sage input preparser

comment:52 Changed 4 weeks ago by git

  • Commit changed from 6456a2af2ba559f2c9766abd8cdcb94eb3597f70 to 5e59f1287d35e85576ef554518fb9f311bbcc9f2

Branch pushed to git repo; I updated commit sha1. New commits:

5e59f12Add missing doctests

comment:53 follow-up: Changed 4 weeks ago by gh-kliem

quote_state needs not to be a global in interpreter.py.

The problem here is that automagic will still have the 3 token replacement. I'm not on your new branch, but you will see a failure similar to

sage: ls | grep \f
/bin/bash: -c: line 0: syntax error near unexpected token `('
/bin/bash: -c: line 0: `ls -F --color | grep * BackslashOperator() * f'

On develop this works just fine.

So the token replacements should not be applied in case of single lines, but then need to be added to SagePreparseTransformer again (to be applied to single lines). We could also remove the backslash token and put it back into SagePreparseTransformer, but this wouldn't fix the following:

Now:

sage: del maxima
sage: maxima f(x)=2
__tmp__=var("x")

Before:

sage: del maxima
sage: maxima f(x)=2
f(x)=2

comment:54 in reply to: ↑ 53 ; follow-up: Changed 4 weeks ago by klee

Replying to gh-kliem:

The problem here is that automagic will still have the 3 token replacement. I'm not on your new branch, but you will see a failure similar to

sage: ls | grep \f
/bin/bash: -c: line 0: syntax error near unexpected token `('
/bin/bash: -c: line 0: `ls -F --color | grep * BackslashOperator() * f'

On develop this works just fine.

So the token replacements should not be applied in case of single lines, but then need to be added to SagePreparseTransformer again (to be applied to single lines). We could also remove the backslash token and put it back into SagePreparseTransformer, but this wouldn't fix the following:

Now:

sage: del maxima
sage: maxima f(x)=2
__tmp__=var("x")

Before:

sage: del maxima
sage: maxima f(x)=2
f(x)=2

These works fine with the branch, fortunately.

quote_state needs not to be a global in interpreter.py.

Would you fix this while you examine the new branch?

comment:55 Changed 4 weeks ago by klee

Let me remind you that the new branch is based on sage beta 9.5.beta2.

comment:56 in reply to: ↑ 54 Changed 4 weeks ago by gh-kliem

Replying to klee:

Replying to gh-kliem:

The problem here is that automagic will still have the 3 token replacement. I'm not on your new branch, but you will see a failure similar to

sage: ls | grep \f
/bin/bash: -c: line 0: syntax error near unexpected token `('
/bin/bash: -c: line 0: `ls -F --color | grep * BackslashOperator() * f'

On develop this works just fine.

So the token replacements should not be applied in case of single lines, but then need to be added to SagePreparseTransformer again (to be applied to single lines). We could also remove the backslash token and put it back into SagePreparseTransformer, but this wouldn't fix the following:

Now:

sage: del maxima
sage: maxima f(x)=2
__tmp__=var("x")

Before:

sage: del maxima
sage: maxima f(x)=2
f(x)=2

These works fine with the branch, fortunately.

Well, because SageTokenTransformers are never added to the token transformers. So they don't do anything.

quote_state needs not to be a global in interpreter.py.

Would you fix this while you examine the new branch?

Yes.

comment:57 Changed 4 weeks ago by git

  • Commit changed from 5e59f1287d35e85576ef554518fb9f311bbcc9f2 to 7cd7482f84b9841f3349046d75cd95d5b6aa7e7d

Branch pushed to git repo; I updated commit sha1. New commits:

0ed2a8cremove global quote_state
7cd7482special treatment of magics not needed anymore

comment:58 Changed 4 weeks ago by git

  • Commit changed from 7cd7482f84b9841f3349046d75cd95d5b6aa7e7d to f1b3439f9204e4ae4fb9c33b47b6f6e8ad959f5f

Branch pushed to git repo; I updated commit sha1. New commits:

f1b3439remove code duplication

comment:59 Changed 4 weeks ago by git

  • Commit changed from f1b3439f9204e4ae4fb9c33b47b6f6e8ad959f5f to 49a9dcbb0d3527a5fcf19d907b6ba25f5ef73a0a

Branch pushed to git repo; I updated commit sha1. New commits:

16e1617remove helper functions
49a9dcbfix 4312 and make doctest stable

comment:60 Changed 4 weeks ago by git

  • Commit changed from 49a9dcbb0d3527a5fcf19d907b6ba25f5ef73a0a to 561764f974923fcd74e7b66ffe29ff957a99d89b

Branch pushed to git repo; I updated commit sha1. New commits:

561764fless differences to develop

comment:61 follow-up: Changed 4 weeks ago by gh-kliem

Let me know, what you think about the changes.

If we do it like this then preparse_calculus and preparse_generators should be deprecated.

As a follow up ticket we could try to have magics in preparse_file, when it is preparsed within ipython.

comment:62 follow-up: Changed 4 weeks ago by gh-kliem

Also closing #4545 could really happen in #4545, as it has nothing to do with our changes. Apparently the problem just went away.

comment:63 Changed 4 weeks ago by git

  • Commit changed from 561764f974923fcd74e7b66ffe29ff957a99d89b to c36e61f43fc1f50b15b8cc7dfbd2bbb25d33c1e1

Branch pushed to git repo; I updated commit sha1. New commits:

c36e61fDeprecate preparse_generators and preparse_calculus

comment:64 in reply to: ↑ 61 Changed 4 weeks ago by klee

Replying to gh-kliem:

Let me know, what you think about the changes.

Looks good.One thing: I am not sure about @parallel. Is it already deprecated?

If we do it like this then preparse_calculus and preparse_generators should be deprecated.

Then let them be deprecated. I made a commit for this.

As a follow up ticket we could try to have magics in preparse_file, when it is preparsed within ipython.

I have no passion for magics in .sage files. But of course you can go ahead.

comment:65 in reply to: ↑ 62 Changed 4 weeks ago by klee

Replying to gh-kliem:

Also closing #4545 could really happen in #4545, as it has nothing to do with our changes. Apparently the problem just went away.

Okay with @parallel.

comment:66 Changed 4 weeks ago by klee

I am positive on the patch.

You can set it positive if you are okay too.

comment:67 Changed 4 weeks ago by git

  • Commit changed from c36e61f43fc1f50b15b8cc7dfbd2bbb25d33c1e1 to 537502fd9352009dea458aefb65e12eca9a13a2d

Branch pushed to git repo; I updated commit sha1. New commits:

537502fleave doctest of 4545 for 4545

comment:68 Changed 4 weeks ago by gh-kliem

  • Authors changed from Jonathan Kliem to Jonathan Kliem, Kwankyu Lee
  • Reviewers changed from Kwankyu Lee to Kwankyu Lee, Jonathan Kliem

From my side this is ok, I would probably wait for a patchbot.

comment:69 Changed 4 weeks ago by git

  • Commit changed from 537502fd9352009dea458aefb65e12eca9a13a2d to 48ab22e2b30c6bc312b8cfaf7bf9eb73d1fd4865

Branch pushed to git repo; I updated commit sha1. New commits:

48ab22eMinor fixes and edits for patchbot complaints

comment:70 follow-up: Changed 4 weeks ago by gh-kliem

This approach has a serious problem though:

https://patchbot.sagemath.org/log/31951/Linux/1_SMP_Debian_5.10.46-5_(2021-09-23)/x86_64/5.10.0-8-amd64/tmonteil-lxc1/2021-09-30%2006:22:01?plugin=startup_modules&diff=/log/0/Linux/1_SMP_Debian_5.10.46-5_%282021-09-23%29/x86_64/5.10.0-8-amd64/tmonteil-lxc1/2021-09-29%2014%3A23%3A05&ticket=31951&base=9.5.beta2

What I understand from this is that now IPython is being loaded, even when just running sage from command line, which is new. I have no idea if this is acceptable.

comment:71 in reply to: ↑ 70 Changed 4 weeks ago by klee

Replying to gh-kliem:

This approach has a serious problem though:

https://patchbot.sagemath.org/log/31951/Linux/1_SMP_Debian_5.10.46-5_(2021-09-23)/x86_64/5.10.0-8-amd64/tmonteil-lxc1/2021-09-30%2006:22:01?plugin=startup_modules&diff=/log/0/Linux/1_SMP_Debian_5.10.46-5_%282021-09-23%29/x86_64/5.10.0-8-amd64/tmonteil-lxc1/2021-09-29%2014%3A23%3A05&ticket=31951&base=9.5.beta2

What I understand from this is that now IPython is being loaded, even when just running sage from command line, which is new. I have no idea if this is acceptable.

It is serious and not acceptable. Let's see if there's an escape.

comment:72 Changed 4 weeks ago by gh-kliem

Yes, I think so. The solution to #30953 is apparently super easy:

-    L = ';%s;' % L.replace('\n', ';\n;')
+    replacements = []
+    counta = 0
+    countb = 0
+    for i in range(len(L)):
+        if L[i] == '[':
+            counta += 1
+        elif L[i] == '(':
+            countb += 1
+        elif L[i] == ']':
+            counta -= 1
+        elif L[i] == ')':
+            countb -= 1
+        elif L[i] == '\n':
+            if counta == countb == 0:
+                replacements.append(i)
+    while replacements:
+        i = replacements.pop()
+        L = L[:i] + ';\n;' + L[i+1:]
+    L = ';' + L
+    if not L[-1:] == ';':
+        L += ';'

I'll push changes, once I tested it (and we can always reset, if we don't like it).

comment:73 Changed 4 weeks ago by git

  • Commit changed from 48ab22e2b30c6bc312b8cfaf7bf9eb73d1fd4865 to 88984334157b63794b4f52c111a48be5a547b8a1

Branch pushed to git repo; I updated commit sha1. New commits:

8898433simpler fix, by replacing only statement ending \n and # with ;\n; and ;#;

comment:74 Changed 4 weeks ago by gh-kliem

Sorry about the mess. This apparently works just with some slight changes in comparison to develop.

comment:75 Changed 4 weeks ago by klee

Why don't we try just to fix the last branch?

It seems that IPython is imported only with 9.5.beta2 + last branch (I mean not with 9.5.beta1). Do you understand how this happen? Is it not surmountable with the approach of the last branch?

comment:76 follow-up: Changed 4 weeks ago by gh-kliem

IPython is surely imported whenever you parse something. Because the token transformer needs IPython.

Whenevern you run

sage -c "print('hello') or similar, this will call preparse, which imports whatever it needs to run. In our case we added the dependency to IPython.

Now the new fix just slightly modified the develop branch to obtain the same output, so I don't know why to use tokens, if a slight modification works just as well.

comment:77 Changed 4 weeks ago by git

  • Commit changed from 88984334157b63794b4f52c111a48be5a547b8a1 to 041aeaadca1846bf8cda58da7f2e3c35267ab23c

Branch pushed to git repo; I updated commit sha1. New commits:

041aeaaA fix

comment:78 in reply to: ↑ 76 Changed 4 weeks ago by klee

Replying to gh-kliem:

IPython is surely imported whenever you parse something. Because the token transformer needs IPython.

Whenevern you run

sage -c "print('hello') or similar, this will call preparse, which imports whatever it needs to run. In our case we added the dependency to IPython.

I see.

Now the new fix just slightly modified the develop branch to obtain the same output, so I don't know why to use tokens, if a slight modification works just as well.

Okay. I agree that the simpler, the better.

All seem work well with me. Let's wait for a green bot.

comment:79 Changed 4 weeks ago by klee

I think the slight increase of the startup time is acceptable. I cannot clearly explain the increase, but I guess this is caused by increasing time of parsing Sage startup scripts.

Otherwise I am positive with the new simpler fix.

comment:80 Changed 4 weeks ago by gh-kliem

  • Status changed from needs_review to positive_review
  • Work issues close #4545 and #30953 deleted

Ok, let's move on.

comment:81 Changed 3 weeks ago by vbraun

  • Branch changed from public/31951 to 041aeaadca1846bf8cda58da7f2e3c35267ab23c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.