-
Notifications
You must be signed in to change notification settings - Fork 21
Setup quantem.widget within monorepo setup, maintain identical uv quantem core dev experience
#141
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
Conversation
bobleesj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvarnavi this is ready for review.
|
Thanks @bobleesj -- this looks good! Reviewed and checked uv workspace works well. However, this doesn't answer the question of whether it solves our js/python bundling issues just yet. It'd be good to add a minimal working react widget to test before we merge this. |
|
@gvarnavi ready for review - PR comment above updated |
gvarnavi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, very clean framework! Left some minor comments for future-proofing.
widget/pyproject.toml
Outdated
| packages = ["src/quantem"] | ||
| artifacts = ["src/quantem/widget/static/*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be?
packages = ["src/quantem/widget"]
artifacts = ["src/quantem/widget/static/**"]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For namespace package, packages = ["src/quantem"], this is correct. I've tested using Test PyPI:
mamba create -n qt-widget-env python=3.12
mamba activate qt-widget-env
pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ quantem.widget
Ref: https://test.pypi.org/project/quantem.widget/
And yes, added ** under artifacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvarnavi Thanks George for the very detailed review. Please see the main commit made here: 8c9206d and it's working after I've uplaoded against that of TestPyPI: https://test.pypi.org/project/quantem.widget/
widget/pyproject.toml
Outdated
| packages = ["src/quantem"] | ||
| artifacts = ["src/quantem/widget/static/*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For namespace package, packages = ["src/quantem"], this is correct. I've tested using Test PyPI:
mamba create -n qt-widget-env python=3.12
mamba activate qt-widget-env
pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ quantem.widget
Ref: https://test.pypi.org/project/quantem.widget/
And yes, added ** under artifacts.
|
Great, thanks! The only remaining thing is to automate the PyPi deployment using the stored |
|
@gvarnavi yeah, I think once we have some useful features in the widget, we can start thinking of deployment things more in details. Created an issue above just to track things and re-visit later. |
What this PR does
Closes #105
Please see the comment #105 (comment) on suggested mono folder structure and benefits
Core quantem dev experience:
Case 1. I'm not a widget developer:
Case 2. I'm a widget developer:
Start developing:
What PR reviewer(s) should do
Please review the PR and merge.
I will add subsequent PRs adding the widgets (react code and GitHub CI next).
Notebook:
widget_demo.ipynb