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

Python package #5

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

zhangir-azerbayev
Copy link

@zhangir-azerbayev zhangir-azerbayev commented Apr 5, 2023

Adds python interface via a package called pylean. See README.md for usage instructions. Unit tests in pylean/tests.

@zhangir-azerbayev
Copy link
Author

Currently, we require users to build the package from source from within this repository. Once we get the compiled version of the repl working, the python package should be installable with only pip install pylean.

Comment on lines +3 to +8

# python gitignore
# Byte-compiled / optimized / DLL files
__pycache__/
*.py[cod]
*$py.class
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not add the kitchen sink, but just what is needed?

Comment on lines +74 to +78
Running Lean programs that `import Mathlib` is a common use case. To enable `mathlib4` imports, simply add the following to `lakefile.lean` and run `lake exe cache get` before running `lake build`.
```
require mathlib from git
"https://github.com/leanprover-community/mathlib4.git"
```
Copy link
Contributor

Choose a reason for hiding this comment

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

People shouldn't be adding a mathlib dependency to the lakefile of this project. Perhaps clarify that you'll need your own lakefile?

print(output)
return json.loads(output)
except pexpect.exceptions.TIMEOUT:
raise pexpect.exceptions.TIMEOUT
Copy link
Member

@eric-wieser eric-wieser Jun 18, 2023

Choose a reason for hiding this comment

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

What's the point in this except? It looks like all it does is make the traceback noisier

self.proc.sendline()
self.proc.expect_exact("\r\n")
try:
index = self.proc.expect('env": \d+\}', timeout=20)
Copy link
Member

Choose a reason for hiding this comment

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

Can you expect the whole line here and parse it as json, rather than parsing using regex?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can't because there are multiline outputs.

There seems to be no guarantee in the Lean code that env will appear at the last, so this is not robust.

Per my test of repl and this comment in the code, repl is using \r\n\r\n as delimiters for requests/responses, just like in HTTP protocol, this is probably the only way to expect here, except to use a loop to expect more lines until the response becomes a valid JSON, which would wastes many JSON parse attempts, or we have to deal with these using fragile regexes.

My implementation inspired by this PR but expecting \r\n\r\n protocol is here, notably, it turns off echo so there is less expecting.

For the stability of parsing repl's output, I hope this \r\n\r\n protocol could be explicitly documented.

self.proc.sendline(command)
self.proc.expect_exact(command + "\r\n")
self.proc.sendline()
self.proc.expect_exact("\r\n")
Copy link
Member

Choose a reason for hiding this comment

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

Does this work on unix? I wouldn't expect there to be an \r there

Copy link
Author

Choose a reason for hiding this comment

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

This works on my ubuntu system.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the \r\n is part of JSON-RPC which is the basis for LSP

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@zhangir-azerbayev
Copy link
Author

Thanks for the reviews.

I see that since I opened this PR, the bug that prevented the repl from compiling was fixed. Therefore I'm going to convert this PR back to a draft and work on a version of the python package that installs a pre-compiled binary of the repl.

@zhangir-azerbayev zhangir-azerbayev marked this pull request as draft June 19, 2023 00:50
Comment on lines +15 to +22
if env:
command = (
json.dumps(dict(cmd=code, env=env))
) # [1:-1] removes single quotes
else:
command = (
'{ "cmd" : "' + repr(code)[1:-1] + '" }'
) # [1:-1] removes single quotes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if env:
command = (
json.dumps(dict(cmd=code, env=env))
) # [1:-1] removes single quotes
else:
command = (
'{ "cmd" : "' + repr(code)[1:-1] + '" }'
) # [1:-1] removes single quotes
data = dict(cmd=code)
if env:
data.update(env=env)
command = json.dump(data)

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.

5 participants