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

Mock the new module API on <38 and ignore second argument. #11593

Merged
merged 7 commits into from Feb 18, 2019

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Feb 5, 2019

As ast.Module is used only in interactiveshell the mock is only a small
lambda function but would need to grow into a proper shim if used more
widely.

@Carreau Carreau added this to the 7.3 milestone Feb 5, 2019
@Carreau
Copy link
Member Author

Carreau commented Feb 5, 2019

@njsmith mind giving it a try ? I'm going to guess ast.changes will will likely have ripple in Jedi as well.

@tacaswell
Copy link
Contributor

What behavior should this effect?

@Carreau
Copy link
Member Author

Carreau commented Feb 5, 2019

None, this should just fix things on 3.8

As ast.Module is used only in interactiveshell the mock is only a small
lambda function but would need to grow into a proper shim if used more
widely.
@tacaswell
Copy link
Contributor

I also run 3.8 on my home machine 😈

@Carreau
Copy link
Member Author

Carreau commented Feb 5, 2019

damn, via conda ? I some point I had a conda recipe that was faking a master as being a 3.6... but it was breaking too often.

@tacaswell
Copy link
Contributor

Via a terrible shell script (https://gist.github.com/tacaswell/a701178de7fcc0c213c0adb0abd57dff ). Basically build cpython, create a venv, pip install into it.

Ah, I now see the issue, kinda hard to miss.... 🐑

@tacaswell
Copy link
Contributor

And yes, this branch fixes it.

@njsmith
Copy link
Contributor

njsmith commented Feb 5, 2019

Change looks good to me. Should you be adding 3.8-dev to your Travis matrix to catch things like this in the future?

@Carreau
Copy link
Member Author

Carreau commented Feb 6, 2019

I've added 38-dev, but as we are failing on nightly with the ast transformers, i'm going to assume it will fail as well, and someone need to investigate why some transformations are not being applied.

@Carreau
Copy link
Member Author

Carreau commented Feb 7, 2019

and so apparently Num has been renamed to Constant in the ast ?

@Carreau
Copy link
Member Author

Carreau commented Feb 7, 2019

Ok, 3 more test fixed. @takluyver, I might need help on some of these errors.

@Carreau
Copy link
Member Author

Carreau commented Feb 7, 2019

It's unclear to me what's the difference between node.n and node.value are they alias to each other now ? If so should one be deprecated ?

It also seem that both Num, and Str are now folded into the Constant ast node.

@Carreau
Copy link
Member Author

Carreau commented Feb 7, 2019

@asmeurer , @williamstein don't you have ast transformers ? If so that will affect you.

…ant ast node

Also fix soem new sytax warnings.
@Carreau
Copy link
Member Author

Carreau commented Feb 7, 2019

Ok, test are passing. I'd love for someone to make sure the code make sens, I wrote that by chunks of 5 min here and there so it might not be super coherent.

@asmeurer
Copy link
Contributor

asmeurer commented Feb 7, 2019

I believe we are still using custom transformers based on tokenize. https://github.com/sympy/sympy/blob/master/sympy/interactive/session.py

def visit_Num(self, node):
node.n = -node.n
return node

if sys.version_info > (3,8):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this if is required (and likewise for several others below). I'd probably just put comments # for python 3.7 and earlier, # for python 3.8+

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review, I added this to every relevant point.


class TokenizeFailureTest(unittest.TestCase):
"""Tests related to https://github.com/ipython/ipython/issues/6864."""

@skipif(sys.version_info > (3,8))
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't review this line because I have no idea why it's here :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

This was apparently testing we properly handle a bug in tokenizer that was fixed in 3.8, so the test is not relevant anymore.

@Carreau Carreau merged commit 542cc5a into ipython:master Feb 18, 2019
@Carreau
Copy link
Member Author

Carreau commented May 31, 2019

Hi there, You seem to run IPython on Python 3.8; do you mind trying #11713 that tried to make use of the new 3.8b1 (not out yet) top-level await ?

@Carreau Carreau deleted the ast-38 branch May 31, 2019 02:08
asfgit pushed a commit to apache/zeppelin that referenced this pull request Sep 28, 2021
### What is this PR for?

The root cause is due to ast api is changed after python 3.8, see ipython/ipython#11593.
* The PR fixed this issue in `zeppelin_python.py`.
* This PR add tests for Python 3.8 for both python interpreter & spark interpreter (spark 2.4 doesn't support python 3.8, so there's no python 3.8 test for spark 2.4).  `jupyter_client` 5 is used because jupyter_client 7 doesn't work (tracked in ZEPPELIN-5533)
* bokeh test is disabled for flink interpreter. Because pyflink's dependencies conflicts with bokeh2.

### What type of PR is it?
[Bug Fix ]

### Todos
* [ ] - Task

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5525

### How should this be tested?
* Ci pass

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jeff Zhang <zjffdu@apache.org>

Closes #4230 from zjffdu/ZEPPELIN-5525 and squashes the following commits:

b816a53 [Jeff Zhang] [ZEPPELIN-5525] Python vanillar interpreter doesn't' work in Python 3.8
asfgit pushed a commit to apache/zeppelin that referenced this pull request Sep 28, 2021
### What is this PR for?

The root cause is due to ast api is changed after python 3.8, see ipython/ipython#11593.
* The PR fixed this issue in `zeppelin_python.py`.
* This PR add tests for Python 3.8 for both python interpreter & spark interpreter (spark 2.4 doesn't support python 3.8, so there's no python 3.8 test for spark 2.4).  `jupyter_client` 5 is used because jupyter_client 7 doesn't work (tracked in ZEPPELIN-5533)
* bokeh test is disabled for flink interpreter. Because pyflink's dependencies conflicts with bokeh2.

### What type of PR is it?
[Bug Fix ]

### Todos
* [ ] - Task

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5525

### How should this be tested?
* Ci pass

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jeff Zhang <zjffdu@apache.org>

Closes #4230 from zjffdu/ZEPPELIN-5525 and squashes the following commits:

b816a53 [Jeff Zhang] [ZEPPELIN-5525] Python vanillar interpreter doesn't' work in Python 3.8

(cherry picked from commit fa1f728)
Signed-off-by: Jeff Zhang <zjffdu@apache.org>
zlosim pushed a commit to zlosim/zeppelin that referenced this pull request Oct 22, 2021
### What is this PR for?

The root cause is due to ast api is changed after python 3.8, see ipython/ipython#11593.
* The PR fixed this issue in `zeppelin_python.py`.
* This PR add tests for Python 3.8 for both python interpreter & spark interpreter (spark 2.4 doesn't support python 3.8, so there's no python 3.8 test for spark 2.4).  `jupyter_client` 5 is used because jupyter_client 7 doesn't work (tracked in ZEPPELIN-5533)
* bokeh test is disabled for flink interpreter. Because pyflink's dependencies conflicts with bokeh2.

### What type of PR is it?
[Bug Fix ]

### Todos
* [ ] - Task

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5525

### How should this be tested?
* Ci pass

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jeff Zhang <zjffdu@apache.org>

Closes apache#4230 from zjffdu/ZEPPELIN-5525 and squashes the following commits:

b816a53 [Jeff Zhang] [ZEPPELIN-5525] Python vanillar interpreter doesn't' work in Python 3.8
zlosim pushed a commit to zlosim/zeppelin that referenced this pull request Oct 22, 2021
### What is this PR for?

The root cause is due to ast api is changed after python 3.8, see ipython/ipython#11593.
* The PR fixed this issue in `zeppelin_python.py`.
* This PR add tests for Python 3.8 for both python interpreter & spark interpreter (spark 2.4 doesn't support python 3.8, so there's no python 3.8 test for spark 2.4).  `jupyter_client` 5 is used because jupyter_client 7 doesn't work (tracked in ZEPPELIN-5533)
* bokeh test is disabled for flink interpreter. Because pyflink's dependencies conflicts with bokeh2.

### What type of PR is it?
[Bug Fix ]

### Todos
* [ ] - Task

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5525

### How should this be tested?
* Ci pass

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jeff Zhang <zjffdu@apache.org>

Closes apache#4230 from zjffdu/ZEPPELIN-5525 and squashes the following commits:

b816a53 [Jeff Zhang] [ZEPPELIN-5525] Python vanillar interpreter doesn't' work in Python 3.8
JasonLeeCoding pushed a commit to JasonLeeCoding/zeppelin that referenced this pull request Jan 5, 2022
### What is this PR for?

The root cause is due to ast api is changed after python 3.8, see ipython/ipython#11593.
* The PR fixed this issue in `zeppelin_python.py`.
* This PR add tests for Python 3.8 for both python interpreter & spark interpreter (spark 2.4 doesn't support python 3.8, so there's no python 3.8 test for spark 2.4).  `jupyter_client` 5 is used because jupyter_client 7 doesn't work (tracked in ZEPPELIN-5533)
* bokeh test is disabled for flink interpreter. Because pyflink's dependencies conflicts with bokeh2.

### What type of PR is it?
[Bug Fix ]

### Todos
* [ ] - Task

### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-5525

### How should this be tested?
* Ci pass

### Screenshots (if appropriate)

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Jeff Zhang <zjffdu@apache.org>

Closes apache#4230 from zjffdu/ZEPPELIN-5525 and squashes the following commits:

b816a53 [Jeff Zhang] [ZEPPELIN-5525] Python vanillar interpreter doesn't' work in Python 3.8
pan3793 pushed a commit to apache/kyuubi that referenced this pull request Nov 11, 2022
### _Why are the changes needed?_

to close #3795

Python ast api is changed after python 3.8, see ipython/ipython#11593

### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [ ] Add screenshots for manual tests if appropriate

- [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #3797 from cfmcgrady/kyuubi-3795.

Closes #3795

44edcea [Fu Chen] Revert "debug python version"
f8171b0 [Fu Chen] Revert "print ga debug info"
16fde4e [Fu Chen] debug python version
331602a [Fu Chen] print ga debug info
66eeb3f [Fu Chen] python 3.8+ support

Authored-by: Fu Chen <cfmcgrady@gmail.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
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

4 participants