Fix CI test suite and and remove Python 2.7 support #508
Conversation
|
Hey @marius311, not sure if you remember this code you wrote back in #262, but this test seems to be failing: def test_local_scope(run_cell):
assert run_cell("""
x = "global"
def f():
x = "local"
ret = %julia py"x"
return ret
f()
""") == "local"I can repeat this locally in a Python REPL and this test also seems to fail: In [26]: %load_ext julia.magic
In [27]: x = "global"
In [28]: def f():
...: x = "local"
...: ret = %julia py"x"
...: return ret
...:
In [29]: f()
Out[29]: 'global'Any idea why this behavior changed? Is this behavior required for some applications, or could this test be deprecated? Thanks! |
|
If I skip that test, everything else passes. So maybe we deprecate that test for now, and fix it in a future PR. @marius311 is that okay with you? |
Codecov ReportBase: 89.20% // Head: 87.34% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #508 +/- ##
==========================================
- Coverage 89.20% 87.34% -1.86%
==========================================
Files 39 39
Lines 2315 2316 +1
==========================================
- Hits 2065 2023 -42
- Misses 250 293 +43
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Not set up to check quickly, but is local variable interp broken entirely? Like does that test work if theres no global x defined, does it fall back to the local? |
|
Seems like it: In [3]: f()
---------------------------------------------------------------------------
NameError Traceback (most recent call last)
Cell In [3], line 1
----> 1 f()
Cell In [2], line 3, in f()
1 def f():
2 x = "local"
----> 3 ret = get_ipython().run_line_magic('julia', 'py"x"')
4 return ret
File ~/venvs/main/lib/python3.11/site-packages/IPython/core/interactiveshell.py:2364, in InteractiveShell.run_line_magic(self, magic_name, line, _stack_depth)
2362 kwargs['local_ns'] = self.get_local_scope(stack_depth)
2363 with self.builtin_trap:
-> 2364 result = fn(*args, **kwargs)
2365 return result
File ~/venvs/main/lib/python3.11/site-packages/julia/magic.py:122, in JuliaMagics.julia(self, line, cell)
118 caller_frame = caller_frame.f_back
120 return_value = "nothing" if src.strip().endswith(";") else ""
--> 122 return self._julia.eval(
123 """
124 _PyJuliaHelper.@prepare_for_pyjulia_call begin
125 begin %s end
126 %s
127 end
128 """
129 % (src, return_value)
130 )(self.shell.user_ns, caller_frame.f_locals)
File PyCall:1
NameError: name 'x' is not defined |
|
Seems like the logic around caller frames changed? # We assume the caller's frame is the first parent frame not in the
# IPython module. This seems to work with IPython back to ~v5, and
# is at least somewhat immune to future IPython internals changes,
# although by no means guaranteed to be perfect.
caller_frame = sys._getframe(3)If I add a |
| while ( | ||
| caller_frame.f_globals.get("__name__").startswith("IPython") | ||
| or caller_frame.f_globals.get("__name__").startswith("julia") | ||
| ): |
There was a problem hiding this comment.
Cool. I think this, and the change to use inspect, fixes it! Turned the test back on.
|
What is with the nightly failures? |
|
I'm not sure. Seems like there's a mismatch in libpython? But not sure how it was generated. |
|
What's going on here: |
Why would this only affect Julia nightly? |
|
Yeah, it is weird. Is it okay if we turn those tests off for now, since they are nightly, and fix them later? |
|
I would rather just keep them there and merge so we do not forget that the nightly tests are failing. |
|
Sounds good to me. |
|
Regarding Python 2.7, is there any other reason to drop it other than it being old, insecure, and deprecated? |
|
That’s the main reason, yeah. Especially since this project is not under active development and only occasional maintenance, I think it would be good to focus any available dev resources on the absolutely critical areas. Although I do think the modification to Julia magic here is only Python 3+ since it uses the inspection module in the Python stdlib (a cleaner way to get the stack frame). |
|
I'm not sure @tkf is active anymore. Is there anybody else who you think would be a good choice for reviewer? |
|
I added |
|
Bug me on Monday November 7th to merge and release. I want to give others some time to respond or review this. |
|
Sending the reminder for today! 🙂 |
|
I need someone with more privileges to fix the CI situation here. @StefanKarpinski ? |
|
I have sufficient permissions to work on this. I'll take a look closer tonight. |
I'll add it back in another commit. The tests need more fixes overall.
|
@MilesCranmer you should have a maintainer role on this repository now. I have admin rights. |
|
Awesome! Thanks |
The fix for the unittests turned out to just be upgrading Python's
coveragetool to 5+, as version 4 was still using HTTP links.@tkf or @mkitti could one of you review this? Once merged this will let us test the other PRs.
This also drops testing for Python 2.7 which is long deprecated and for which all use should be discouraged (since they stopped doing security patches in the end of 2019).