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

Allow Cython compilation to open a web browser containing the annotated HTML file #38946

Merged
merged 12 commits into from
Jan 4, 2025

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Nov 9, 2024

As in the title.

I think this is a rather common use case, to investigate whether the code does get the desired speedup.

Remark: there's sage.misc.viewer.browser() which could be used instead, but then a lot of existing code in SageMath uses webbrowser.open() anyway?

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion. (not aware of one)
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

Dependencies

Depends on #38945 .

Copy link

github-actions bot commented Nov 9, 2024

Documentation preview for this PR (built with commit 6116696; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@user202729 user202729 changed the title Allow passing view_annotate=True to Cython compilation Allow Cython compilation to open a web browser containing the annotated HTML file Nov 9, 2024
@kwankyu
Copy link
Collaborator

kwankyu commented Nov 10, 2024

This looks nice.

It would be nicer if the new argument view_annotate is accepted to the cell magic %%cython. For example,

%%cython view_annotate=True
def f(int n):
...

or

%%cython --view_annotate
def f(int n):
...

The cell magic is defined in https://github.com/sagemath/sage/blob/develop/src/sage/repl/ipython_extension.py. Find the line

def cython(self, line, cell):

line seems to accept the part after "%%cython". So we may use it to provide arguments to the cython function. What do you think?

@user202729
Copy link
Contributor Author

Already did that in #38945 . (though whichever being merged first need to be modified to incorporate the other…)

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 10, 2024

You may set #38945 as a dependency and merge it here.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 10, 2024

and instead of

- ``line`` -- parsed as keyword arguments. See :func:`~sage.misc.cython.cython` for details.

you may at least list the possible arguments for users' convenience, leaving details to sage.misc.cython.cython .

@user202729 user202729 force-pushed the cython-view-annotate branch 2 times, most recently from b8d3e69 to 99bce53 Compare November 14, 2024 15:36
@user202729
Copy link
Contributor Author

I learnt it's possible to view the result in the Sage notebook (if Jupyter-based notebook GUI is used) from looking at cython-provided magic, so I implement that here.

@user202729 user202729 force-pushed the cython-view-annotate branch 2 times, most recently from 269c4d8 to 0447579 Compare December 12, 2024 14:17
@user202729
Copy link
Contributor Author

@kwankyu Can you review the PR? Thanks.

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 23, 2024

Conflicts?

@user202729
Copy link
Contributor Author

user202729 commented Dec 23, 2024

Huh, that only come from the new commits for 10.6.beta2 today. This one is pretty harmless.

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 23, 2024

On comand line (on mac),

sage: %%cython --view-annotate
....: def f(int n):
....:     return n*n
....: 

does not open a web browser.

@user202729
Copy link
Contributor Author

Not sure what's going on, but probably doesn't have anything to do with the code. There are some complaints e.g. https://stackoverflow.com/questions/65981130/python-webbrowser-module-doesnt-work-on-terminal or python/cpython#69143

You can try oeis.browse() which also uses webbrowser.open() — if it also doesn't work it probably isn't the program's fault.

If it does work, maybe you can try this patch

diff --git a/src/sage/misc/cython.py b/src/sage/misc/cython.py
index fde36372712..3689c896b4a 100644
--- a/src/sage/misc/cython.py
+++ b/src/sage/misc/cython.py
@@ -81,7 +81,7 @@ sequence_number = {}
 
 def cython(filename, verbose=0, compile_message=False,
            use_cache=False, create_local_c_file=False, annotate=True, view_annotate=False,
-           view_annotate_callback=webbrowser.open, sage_namespace=True, create_local_so_file=False):
+           view_annotate_callback=lambda x: webbrowser.open("file://" + x), sage_namespace=True, create_local_so_file=False):
     r"""
     Compile a Cython file. This converts a Cython file to a C (or C++ file),
     and then compiles that. The .c file and the .so file are

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 23, 2024

If it does work, maybe you can try this patch

diff --git a/src/sage/misc/cython.py b/src/sage/misc/cython.py
index fde36372712..3689c896b4a 100644
--- a/src/sage/misc/cython.py
+++ b/src/sage/misc/cython.py
@@ -81,7 +81,7 @@ sequence_number = {}
 
 def cython(filename, verbose=0, compile_message=False,
            use_cache=False, create_local_c_file=False, annotate=True, view_annotate=False,
-           view_annotate_callback=webbrowser.open, sage_namespace=True, create_local_so_file=False):
+           view_annotate_callback=lambda x: webbrowser.open("file://" + x), sage_namespace=True, create_local_so_file=False):
     r"""
     Compile a Cython file. This converts a Cython file to a C (or C++ file),
     and then compiles that. The .c file and the .so file are

Yes. That does the trick. For readability, you may want to store the lambda function to a variable first, before line 82.

@user202729
Copy link
Contributor Author

I come across #33931 , while it looks like the PR is stalled it is recommended to use webbrowser over sage.misc.viewer. So I'll use webbrowser.open() anyway.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 1, 2025
…aining the annotated HTML file

    
As in the title.

I think this is a rather common use case, to investigate whether the
code does get the desired speedup.

Remark: there's `sage.misc.viewer.browser()` which could be used
instead, but then a lot of existing code in SageMath uses
`webbrowser.open()` anyway?

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion. (not aware of one)
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.

### Dependencies

Depends on sagemath#38945 .
    
URL: sagemath#38946
Reported by: user202729
Reviewer(s): Kwankyu Lee, user202729
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 1, 2025
    
* for some reason the proper header is needed to open the HTML file on
my machine.
* It's reported that `webbrowser.open()` doesn't work when called on a
file path, rather an URI is needed
(sagemath#38946)


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (not needed)

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39193
Reported by: user202729
Reviewer(s): Martin Rubey
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 3, 2025
    
* for some reason the proper header is needed to open the HTML file on
my machine.
* It's reported that `webbrowser.open()` doesn't work when called on a
file path, rather an URI is needed
(sagemath#38946)


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (not needed)

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39193
Reported by: user202729
Reviewer(s): Martin Rubey
@vbraun vbraun merged commit a796b44 into sagemath:develop Jan 4, 2025
22 of 24 checks passed
@antonio-rojas
Copy link
Contributor

A test is failing here

**********************************************************************
File "/usr/lib/python3.13/site-packages/sage/repl/ipython_extension.py", line 433, in sage.repl.ipython_extension.SageMagics.cython
Failed example:
    shell.run_cell('''
    %%cython --view-annotate=xx
    print(1)
    ''')
Expected:
    UsageError: argument --view-annotate: invalid choice: 'xx' (choose from 'none', 'auto', 'webbrowser', 'displayhtml')
Got:
    UsageError: argument --view-annotate: invalid choice: 'xx' (choose from none, auto, webbrowser, displayhtml)
**********************************************************************

@user202729
Copy link
Contributor Author

Should be fixed in #39282

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 5, 2025
Fixes issue pointed out in
sagemath#38946 (comment)

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

URL: sagemath#39282
Reported by: user202729
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 7, 2025
    
Fixes issue pointed out in
sagemath#38946 (comment)

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39282
Reported by: user202729
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 9, 2025
    
Fixes issue pointed out in
sagemath#38946 (comment)

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39282
Reported by: user202729
Reviewer(s): Frédéric Chapoton
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 10, 2025
    
Fixes issue pointed out in
sagemath#38946 (comment)

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39282
Reported by: user202729
Reviewer(s): Frédéric Chapoton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants