OK Debugger: Python Tutor Tracing #301
Conversation
* master: Include bump to v1.11.1 remove unnecessary import in config.py (#300)
|
Update: It works for project too but there's a little bit of work to do in hiding irrelevant information.
|
| modules = {k.replace('.py', '').replace('/', '.'): v for k,v in messages['file_contents'].items()} | ||
| data = generate_trace.run_logger(test_script, setup, modules) or "{}" | ||
| messages['tracing']['end-trace'] = get_time() | ||
| messages['tracing']['trace-len'] = len(json.loads(data).get('trace', [])) # includes the code since data is a str |
There was a problem hiding this comment.
If the trace gets huge - we probably don't want to be loading/dumping all over the place - but this should be fine.
| return | ||
|
|
||
| test = tests[0] | ||
| data = test.get_code() |
There was a problem hiding this comment.
Should probably test that your test case has a get_code attr (since it's not actually part of the test base model) - we might want to add it there - (and just have it return {} - or throw an exception and catch it here)
| if data: | ||
| messages['tracing']['start-server'] = get_time() | ||
| # Open Python Tutor Browser Window with this trace | ||
| server.run_server(data) |
There was a problem hiding this comment.
--trace-print should just print the contents of the trace - we should probably have it accept a file argument to dump to instead of stdout (since a bunch of other stuff gets printed to stdout too)
cc: @epai (just fyi - it could make your life easier later)
|
@epai - Ping on merging this? I don't think any of my comments are blockers. |
|
No objections on my end, I'd suggest giving a quick trial run |
|
Some tasks before merge:
|
|
@Sumukh can you take a look at my changes? I removed unused global variables, using the |
|
That’s awesome! I’ll take a look soon
…On Sun, Dec 22, 2019 at 4:36 AM Kavi Gupta ***@***.***> wrote:
@Sumukh <https://github.com/Sumukh> can you take a look at my changes? I
removed unused global variables, using the ast_scope module I've been
developing for the last month or so (
https://github.com/kavigupta/ast_scope)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#301?email_source=notifications&email_token=AAGXNTKNFMJB54RNTUKWDVTQZ5NN7A5CNFSM4DKMEPN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHPO6GY#issuecomment-568258331>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGXNTIDJN4KALOE7PSMQE3QZ5NN7ANCNFSM4DKMEPNQ>
.
|
|
@kavigupta Your changes overall looks good to me. I'm a bit concerned about what including networkx does to the ok client bundle size but maybe the impact is smaller than I would imagine |
|
oh wow it goes from 2.0MB to 6.4MB. I think I'll change ast_scope to remove the networkx dependency, we're really only using it for a DFS |
|
@Sumukh removed the dependency on |
| for neighbor in graph.neighbors(node): | ||
| helper(neighbor) | ||
| helper(node) | ||
| return seen |
There was a problem hiding this comment.
Who said you'll never have to write a DFS in the real world ;)
|
Nice! I haven't really tried the actual result yet but feel free to merge & deploy! |
|
ok sounds good! |
|
@kavigupta I think this broke OkPy. Did you forget to add the new dependencies to pip requirement (or elsewhere)? I'm getting errors about |
This PR brings in a debugger into OK (with the help of a slightly modified version of @pgbovine's Python Tutor)
Usage:
python3 ok -q q1 --tracepython3 ok -q q1 --trace --suite 3I haven't really tried this on projects yet.