Add subscription block #348
Conversation
Codecov Report
@@ Coverage Diff @@
## master #348 +/- ##
=========================================
+ Coverage 72.43% 72.53% +0.1%
=========================================
Files 312 316 +4
Lines 23330 24676 +1346
=========================================
+ Hits 16899 17899 +1000
- Misses 6431 6777 +346 |
|
Indeed this looks handy, I will try to think about corner cases and review carefully later. I would also like to see some systematic tests that we do track state properly (in particular in the presence of intermediate assignment), an addition to the documentation and to the changelog. @sccolbert I am always interested in your opinion. |
|
We added something like this to our internal fork of enaml too - it is a decorator that can be applied to any function in the stack, causing it to be ‘traced’. Handy for complex bits and bobs that came up now and then, and similar in intent to above. Makes sense. |
|
One thing I don't like is the lack of a |
|
Yeah - agreed - it does have a slightly strange feel to it... but a lot of enaml is something to get used to anyway. It’s close enough to Python for that to not be too hard on the eyes. And it’s an appealing syntax in that it matches the current 1 line expression. |
| @@ -670,7 +670,7 @@ def annotate_indentation_state(self, token_stream): | |||
| # by suppressing NEWLINE tokens in a multiline expression. | |||
| # So, we can treat double colon the same as colon here for | |||
| # handling the logic surrounding indentation state. | |||
| if token.type in ("COLON", "DOUBLECOLON"): | |||
| if token.type in ("COLON", "DOUBLECOLON", "LEFTSHIFT"): | |||
There was a problem hiding this comment.
This may cause issues with bitshift operators. I will have to think about it.
There was a problem hiding this comment.
Good point. I added some tests to try and check for this. It appears to be working correctly but I might not be catching the case(s) in which this may occur.
|
I want to think about and comment on this before it's merged.
…On Mon, Apr 8, 2019, 9:31 PM Matthieu Dartiailh ***@***.***> wrote:
***@***.**** commented on this pull request.
Apart from one possible (I need to check) issue with bitwise operator.
This looks good. As I already mentioned I would like to see test and
documentation before merging.
------------------------------
In enaml/core/parser/base_lexer.py
<#348 (comment)>:
> @@ -670,7 +670,7 @@ def annotate_indentation_state(self, token_stream):
# by suppressing NEWLINE tokens in a multiline expression.
# So, we can treat double colon the same as colon here for
# handling the logic surrounding indentation state.
- if token.type in ("COLON", "DOUBLECOLON"):
+ if token.type in ("COLON", "DOUBLECOLON", "LEFTSHIFT"):
This may cause issues with bitshift operators. I will have to think about
it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#348 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAIYSQLkni01yXIP7W21GVOvlYO3rDcGks5ve_uDgaJpZM4cfzm7>
.
|
|
Sure. Usually I try to give you a long notification before merging anything. |
|
Bump... any updates? |
|
I discussed some time ago with @sccolbert and his take what to first check if we could not do the same with a declarative function. He though that it would be traced automatically. Turned out it is not the case. |
|
Sorry for the delay. I discussed this a bit more with @sccolbert and he fears that we may be missing something by allowing assignments in a traced code. Basically he wants to be sure we cover everything single corner case and make sure we are restrictive enough in what is allowed to avoid bad surprises later. I have been working on other projects I am also a maintainer for lastly hence the slow progress. I will try to put more time in this and test this as thoroughly as I can. I will push updated tests as I go. |
|
No rush.
Could you explain what the potential concerns are with this? If this is a problem then assignment expressions from 3.8 will also be an issue. |
|
@sccolbert was not very clear on this point sadly. Python 3.8 will be a pain because of assignment expression syntax and also some changes to the bytecode too ! I have not yet started working on it. I would prefer to do more work on Atom but I am making only slow progress. |
|
@tstordyallison when adding your decorator did you encounter any issue ? Would you consider making a PR so that I can have a look at it ? It may be an interesting alternative (at least I would like to have a look). |
|
Sure - will try and dig it out and see what I can do. Will probably be a week or so till I can get to it though (on vacation!). We are embarking on a Python 3 upgrade right now, so someone (probably not me) will have to merge it in our fork soon at the very least anyway... |
|
Thanks @tstordyallison. Out of curiosity why did you choose to maintain your own branch ? |
|
No good reasons - the original fork was done into SVN with no history and had many edits on top when I first got to it; the project was seemingly dead in OSS at the height of our internal activity; OSS contributions for employees were gnarly to get approved at the time. With our move to Python 3, we are coming back onto the mainline - our 3rd party library fork story is much better (and we are on git nowadays), the project is pretty alive (thanks!) and our OSS story is a well trodden path. |
|
Also - I’m super keen for all the coverage and tests that have been added since too! |
|
Still a lot to do on that front but yes it is getting better. |
|
@frmdstryr Rebasing on master should fix the build issue. |
MatthieuDartiailh
force-pushed
the
subscription-block
branch
from
a4d6ecf
to
729577d
Compare
|
The history of that branch was really weird and my additional tests had disappeared so I rebased and force pushed. |
|
With If I fall back to my original master branch that has this merged in, these segfaults don't occur. It seems to be related to #400, the segfault is happening when items in the Looper are being replaced. It's segfaulting in DynamicScope_getitem: Thread 1 "python" received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
(gdb) bt
#0 0x0000000000000000 in ?? ()
#1 0x000055555566bccd in PyDict_GetItem (op=0x7ffff7fb8910, key=0x7ffff6a87a70) at /tmp/build/80754af9/python_1572016129546/work/Objects/dictobject.c:1327
#2 0x00007fffde567acd in enaml::(anonymous namespace)::DynamicScope_getitem(enaml::DynamicScope*, _object*) ()
from /home/jrm/Workspace/enaml/enaml/core/dynamicscope.cpython-37m-x86_64-linux-gnu.so
#3 0x00005555556844a9 in PyObject_GetItem (o=0x7fffb892b750, key=<optimized out>) at /tmp/build/80754af9/python_1572016129546/work/Objects/abstract.c:154
#4 0x0000555555724bb1 in _PyEval_EvalFrameDefault (f=<optimized out>, throwflag=<optimized out>)
at /tmp/build/80754af9/python_1572016129546/work/Python/ceval.c:2071
#5 0x00005555556671b9 in _PyEval_EvalCodeWithName (_co=0x7fffb9616b70, globals=<optimized out>, locals=<optimized out>, args=<optimized out>,
argcount=<optimized out>, kwnames=0x0, kwargs=0x0, kwcount=<optimized out>, kwstep=2, defs=0x0, defcount=0, kwdefs=0x0, closure=0x0, name=0x0,
qualname=0x0) at /tmp/build/80754af9/python_1572016129546/work/Python/ceval.c:3930
#6 0x0000555555668094 in PyEval_EvalCodeEx (_co=<optimized out>, globals=<optimized out>, locals=<optimized out>, args=<optimized out>,
argcount=<optimized out>, kws=<optimized out>, kwcount=0, defs=0x0, defcount=0, kwdefs=0x0, closure=0x0)
at /tmp/build/80754af9/python_1572016129546/work/Python/ceval.c:3959
#7 0x00007ffff1631313 in enaml::call_func(_object*, _object*) () from /home/jrm/Workspace/enaml/enaml/core/funchelper.cpython-37m-x86_64-linux-gnu.so
#8 0x00005555556b7bb0 in _PyMethodDef_RawFastCallKeywords (method=0x7ffff16340c0 <enaml::(anonymous namespace)::funchelper_methods>, self=0x7fffd89db6b0,
args=0x555558595f08, nargs=<optimized out>, kwnames=<optimized out>) at /tmp/build/80754af9/python_1572016129546/work/Objects/call.c:698
#9 0x00005555556b7d51 in _PyCFunction_FastCallKeywords (func=0x7fffd8e3aeb0, args=<optimized out>, nargs=<optimized out>, kwnames=<optimized out>)
at /tmp/build/80754af9/python_1572016129546/work/Objects/call.c:734
#10 0x0000555555723974 in call_function (kwnames=0x0, oparg=4, pp_stack=<synthetic pointer>)
at /tmp/build/80754af9/python_1572016129546/work/Python/ceval.c:4568I'll see If I can make an example to reproduce it but the code is roughly: Looper:
iterable << some_items
Td:
attr availability << is_available(loop_item, schedule)
attr block << availability[0].block if availability else None
# Works fine if I push the expr into a func
#text << format_text(loop_item, schedule, block)
text <<
t = get_count(loop_item, schedule)
if block:
t += f' {block.description}' # It is blowing up at this line
return t
# This also segfaults
#text << get_count(loop_item, schedule) + (
# f' {block.description}' if block else '')If I replace the When using the subscription block method fauthandler is telling me it's failing at the It seems like the |
|
That looks nasty ! If you can get a self-contained reproducer I will try to investigate. |
|
Ok, well the backtrace is completely different when using it in Qt but I can still get it to segfault with this: import faulthandler
faulthandler.enable()
import uuid
import random
from atom.api import *
from enaml.core.api import Looper
from enaml.widgets.api import Window, Container, Form, Label, PushButton
class Block(Atom):
description = Str()
class Availability(Atom):
block = Typed(Block)
enamldef Main(Window): window:
func generate_rows():
rows = []
for i in range(random.randint(0, 10)):
rows.append(uuid.uuid4())
return rows
attr rows = generate_rows()
attr extra_columns = ['Test']
func get_availability(slot, column):
if random.randint(0, 4) > 1:
return False
return Availability(block=Block(description=str(slot)))
func format_item(row, column, block):
return str(column)
Container:
PushButton:
text = "Refresh"
clicked :: window.rows = generate_rows()
Looper:
iterable << rows
Form:
attr row = loop.item
Label:
text = "Row %s" % loop.index
# If this looper is commened out it seems to work fine
Looper:
iterable << extra_columns
Label:
attr availability << get_availability(row, loop.item)
attr block << availability.block if availability else None
text << format_item(row, loop.item, block)# + f' {block.description}' if block else ''Just click refresh a bunch of times. If the inner looper is commented out I can't get it to segfault at all. It doesn't seem to matter if the commented out part of the last line is there or not. hread 1 "python" received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
(gdb) bt
#0 0x0000000000000000 in ?? ()
#1 0x00005555556b8310 in _PyDict_LoadGlobal (globals=0x7fffeedb17c0, builtins=0x7ffff7f44c80, key=0x7ffff7f47cb0) at /home/conda/feedstock_root/build_artifacts/python_1587713251558/work/Objects/dictobject.c:1509
#2 0x00005555557403e6 in _PyEval_EvalFrameDefault (f=<optimized out>, throwflag=<optimized out>) at /home/conda/feedstock_root/build_artifacts/python_1587713251558/work/Python/ceval.c:2505
#3 0x00005555556e7049 in _PyEval_EvalCodeWithName (_co=0x7fffeedacf50, globals=<optimized out>, locals=<optimized out>, args=<optimized out>, argcount=<optimized out>, kwnames=0x0, kwargs=0x7fffffffc460, kwcount=<optimized out>, kwstep=1, defs=0x0,
defcount=0, kwdefs=0x0, closure=0x7fffeeddc7c0, name=0x7fffeeddba70, qualname=0x7fffeedda8f0) at /home/conda/feedstock_root/build_artifacts/python_1587713251558/work/Python/ceval.c:4298
#4 0x00005555556e7c0e in _PyFunction_Vectorcall (func=<optimized out>, stack=0x7fffffffc458, nargsf=<optimized out>, kwnames=<optimized out>) at /home/conda/feedstock_root/build_artifacts/python_1587713251558/work/Objects/call.c:435
#5 0x00005555555ebef7 in _PyObject_Vectorcall (kwnames=0x0, nargsf=1, args=0x7fffffffc458, callable=0x7fffeeda5d30) at /home/conda/feedstock_root/build_artifacts/python_1587713251558/work/Include/cpython/abstract.h:127
#6 method_vectorcall (method=<optimized out>, args=0x7ffff7f77058, nargsf=<optimized out>, kwnames=<optimized out>) at /home/conda/feedstock_root/build_artifacts/python_1587713251558/work/Objects/classobject.c:67
#7 0x000055555569b82f in PyVectorcall_Call (callable=0x7ffff57b3800, tuple=<optimized out>, kwargs=<optimized out>) at /home/conda/feedstock_root/build_artifacts/python_1587713251558/work/Objects/call.c:199
#8 0x00007ffff15f2c14 in call_method (va=0x7fffffffc528, fmt=0x7ffff11c68e5 "", method=0x7ffff57b3800) at siplib.c:2264
#9 sip_api_call_method (isErr=0x0, method=0x7ffff57b3800, fmt=0x7ffff11c68e5 "") at siplib.c:2319
#10 0x00007ffff1079aa3 in sipVH_QtWidgets_10(PyGILState_STATE, void (*)(_sipSimpleWrapper*, PyGILState_STATE), _sipSimpleWrapper*, _object*) () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/QtWidgets.abi3.so
#11 0x00007ffff11ae2c4 in sipQLayout::minimumSize() const () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/QtWidgets.abi3.so
#12 0x00007ffff0761bed in QLayout::totalMinimumSize() const () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/Qt/lib/libQt5Widgets.so.5
#13 0x00007ffff07629e8 in QLayout::activate() () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/Qt/lib/libQt5Widgets.so.5
#14 0x00007ffff0743ad7 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/Qt/lib/libQt5Widgets.so.5
#15 0x00007ffff074aa90 in QApplication::notify(QObject*, QEvent*) () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/Qt/lib/libQt5Widgets.so.5
#16 0x00007ffff11ae74e in sipQApplication::notify(QObject*, QEvent*) () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/QtWidgets.abi3.so
#17 0x00007ffff44cec48 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/Qt/lib/libQt5Core.so.5
#18 0x00007ffff44d21da in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/Qt/lib/libQt5Core.so.5
#19 0x00007ffff45280a3 in postEventSourceDispatch(_GSource*, int (*)(void*), void*) () from /home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/PyQt5/Qt/lib/libQt5Core.so.5Without running in gdb faulthandler says: Traceback (most recent call last):
File "/home/jrm/miniconda3/envs/py38/lib/python3.8/site-packages/enaml/qt/qt_container.py", line 507, in _on_relayout_timer
File "/home/jrm/miniconda3/envs/py38/lib/python3.8/contextlib.py", line 240, in helper
File "/home/jrm/miniconda3/envs/py38/lib/python3.8/contextlib.py", line 86, in __init__
NameError: name 'getattr' is not defined
Unhandled Python exception
Fatal Python error: AbortedJust for some context, the actual use case for using two loopers to dynamically build a table that has columns that change based on various options. Edit: This might be a bug with PyQt5 5.14 because I'm getting random crashes in completely unrelated code. |
|
So finally managed to find some time for this and bad news I can reproduce but only on 0.11. I played around and patched 0.10.4 to work on atom 0.5.0 and I cannot crash it. Which means we have a weird corruption issue somewhere in enaml c++ code. I need to refresh more than 10 times to get to the the crash so finding what corrupt the builtins is going to be a pain. Anybody willing to help me track this bug is welcome. |
|
Ok thanks for confirming this, I'll see if I can find anything. Should I create an issue? |
|
I resolved the issue I will push the fix and make a bugfix release. |
|
I will try to do a bugfix release sometimes next week, do not hesitate to ping me if necessary. |
|
0.11.2 contains the fix |
|
@sccolbert did you give this any though ? I tried to add test for the cases I could think about and it seems reasonable to me. @tstordyallison did you ever manage to dig the decorator code you mentioned in that thread ? |
|
Since your fix was added I haven't run into any issues with this. |
MatthieuDartiailh
force-pushed
the
subscription-block
branch
from
729577d
to
4f04a34
Compare
|
Finally took some time to revisit this and added some missing Python 3.8 features on top (the walrus operator in particular). Including the walrus does not pause any grammatical issue, and since the left hand side is limited to single name it is even less likely to cause issue than the subscription block. @tstordyallison I am still interested if you can dig up your decorator at one point. I will try to convince myself that the tests are sufficient within a week or two. I hope to merge this no later than September 1st. Please try to break it since I would prefer to find any issue now rather than after merging. |
MatthieuDartiailh
force-pushed
the
subscription-block
branch
from
a8886da
to
8a061ac
Compare
|
For me this is good to go. @frmdstryr since this was your work originally could you have a last look at the tests and see if I may have missed anything ? |
|
Looks like this expanded a bit from the original PR... can the stuff after affd44c be moved to a separate PR? |
|
Sure I can move it. I added it because working on the walrus here made some sense at the beginning. |
MatthieuDartiailh
force-pushed
the
subscription-block
branch
from
568b3e8
to
c42d29f
Compare
Codecov Report
@@ Coverage Diff @@
## master #348 +/- ##
==========================================
+ Coverage 73.35% 73.53% +0.18%
==========================================
Files 311 311
Lines 23788 23815 +27
==========================================
+ Hits 17449 17513 +64
+ Misses 6339 6302 -37 |
|
Are you fine with that commit set @frmdstryr ? I moved the rest to #422 |
|
Yes looks good |
Well that was easy.