Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

Bring (py)Chiquito in #86

Merged
merged 8 commits into from
Aug 21, 2023
Merged

Bring (py)Chiquito in #86

merged 8 commits into from
Aug 21, 2023

Conversation

gnart33
Copy link
Contributor

@gnart33 gnart33 commented Aug 18, 2023

Resolve #82

  • Add scr/python.rs contains Python Module
  • frontend/python/chiquito is home for python-source
  • dependencies = ["py_ecc"] included in pyproject.toml so users don't have to pip install py_ecc
  • add fibonacci.py to examples

@qwang98
Copy link
Collaborator

qwang98 commented Aug 18, 2023

This is also to confirm with @leolara, but my understanding is that Chiquito Python code (non examples) should be in src/frontend/python/chiquito rather than frontend/python/chiquito? Then pychiquito.rs and maturin bindings in python.rs in this branch should go into src/frontend/python.

.gitignore Outdated
@@ -1,3 +1,6 @@
/target
/Cargo.lock
.vscode
.env
__pycache__
*.so
Copy link
Collaborator

Choose a reason for hiding this comment

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

Entirely cosmetic but we typically add one empty line to end of file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cosmetic haha

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, in some edge cases it can lead to unnecessary git conflicts when several people changes the last lines of the file. So we have this rule of always ending all files with an empty line

README.md Outdated
Comment on lines 51 to 66
### Install chiquito with pip

```bash
pip install chiquito
```

### Build from source

In the root of this repo run

```bash
python -m venv .env
source .env/bin/activate
pip install maturin
maturin develop
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding this!

Are "install chiquito with pip" and "build from source" considered two different methods for installation or two steps of installation?

To confirm, after pip install chiquito, is rust_chiquito directly accessible from chiquito.rust_chiquito or requires maturin develop? My guess is the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's either to install, pip package will be available for python user, maturin develop is only for who want to build from source/develop.
Will make it clear

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that I've updated the example in the PyChiquito repo, but we can focus on getting the structure working before porting over the latest PyChiquito updates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self: there are also some python comments that I should delete once we have this merged.

src/python.rs Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, my understanding is that this file plus pychiquito.rs should go into src/frontend/python/mod.rs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that is the spec of the task, if that is possible, i would like it to be there

@qwang98 qwang98 requested a review from leolara August 18, 2023 05:08
@gnart33
Copy link
Contributor Author

gnart33 commented Aug 18, 2023

Let wait for Leo to reach the agreement for the structure then I will reorg accordingly.

@leolara
Copy link
Collaborator

leolara commented Aug 18, 2023

the content of frontend/pychiquito.rs together with this PR src/pyton.rs should be in frontend/python/mod.rs

@leolara
Copy link
Collaborator

leolara commented Aug 18, 2023

This is also to confirm with @leolara, but my understanding is that Chiquito Python code (non examples) should be in src/frontend/python/chiquito rather than frontend/python/chiquito? Then pychiquito.rs and maturin bindings in python.rs in this branch should go into src/frontend/python.

Yes, when I have said frontend/python, I meant src/frontend/python

@leolara
Copy link
Collaborator

leolara commented Aug 18, 2023

It was my mistake, I put it wrong in the issue, I have corrected it now.

@qwang98
Copy link
Collaborator

qwang98 commented Aug 21, 2023

FYI, we should also add a tutorial folder that contains all the Jupyter files.

@leolara
Copy link
Collaborator

leolara commented Aug 21, 2023

FYI, we should also add a tutorial folder that contains all the Jupyter files.

👍

@gnart33
Copy link
Contributor Author

gnart33 commented Aug 21, 2023

Dont merge yet but please see how it look

@leolara
Copy link
Collaborator

leolara commented Aug 21, 2023

@trangnv we usually let the contributor to merge once approved

@leolara
Copy link
Collaborator

leolara commented Aug 21, 2023

@trangnv I think it is missing:

  • move src/frontend/pychiquito.rs content to src/frontend/python/mod.rs ; src/frontend/pychiquito.rs should be removed
  • move jupyter tutorials
  • Perhaps add a make build that builds the rust and the python

Copy link
Collaborator

@leolara leolara left a comment

Choose a reason for hiding this comment

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

Excellent!!

@leolara
Copy link
Collaborator

leolara commented Aug 21, 2023

@trangnv It is approved, merge it please

@gnart33 gnart33 added this pull request to the merge queue Aug 21, 2023
Merged via the queue into main with commit ecc80f1 Aug 21, 2023
1 check was pending
@gnart33
Copy link
Contributor Author

gnart33 commented Aug 21, 2023

@trangnv It is approved, merge it please

👍

@alxkzmn alxkzmn deleted the trang/chiquito-python-frontend-1 branch August 7, 2024 11:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moving PyChiquito to the main repo
3 participants