Support for Automatic <py-env> with dependency + few tweaks to contributing and readme docs
This PR finalises the new pyscript-cli features developed during the last hack-days.
In particular, the new features this PR brings are:
- support for automatic analysis of code dependencies (i.e. imports) which will be then injected into a
<py-env>tag in the html template.- both packages and (local) modules are supported - resulting in
pathsas per PyScript naming. - Unsupported imports and packages (e.g. packages not supported by Pyodide) are also detected, with a WARNING raised to signal that resulting code may not work.
- both packages and (local) modules are supported - resulting in
- now both Python files (
.py) and Notebooks (.ipynb) could be passed in as input and converted.- so far notebooks are merely converted into Python files before being processed.
Note: from hack-days version, this code has been merged and aligned to support the new plugin-hook design/feature, plus - more importantly - a few tests for the new features have been included.
On this note, a quick refactoring has been done to test_cli to include fixtures for code, with relatively simple injectable dependencies.
Other changes in this PR concerns:
- improved
CONTRIBUTING.mddocumentation providing instructions on how to setup dev environment (not just for docs)- on this note,
poetrydependencies have been revised and updated, includingpre-commitas dependency, with instructions in the main doc on how to install hooks.
- on this note,
- added line in
README.mdto showcase input code from both python and notebooks files.
@leriomaggio Sorry, I have forgotten to configure isort to prevent conflicts with black during pre-commit. Can you please add the following to pyproject.toml?
[tool.isort]
profile = "black"
@leriomaggio Sorry, I have forgotten to configure
isortto prevent conflicts withblackduringpre-commit. Can you please add the following topyproject.toml?[tool.isort] profile = "black"
YES! that was giving me headaches already!! will do :D
@mattkram Any idea on why the tests are failing ? - quickly looking on the internet it seems an issue with poetry version? but not sure.. REF
@mattkram Any idea on why the tests are failing ? - quickly looking on the internet it seems an issue with poetry version? but not sure.. REF
Tried to update poetry-core dep to see if that fixes the issue
(as per instructions from poetry#4983
@mattkram Any idea on why the tests are failing ? - quickly looking on the internet it seems an issue with poetry version? but not sure.. REF
Tried to update
poetry-coredep to see if that fixes the issue(as per instructions from poetry#4983
I'm not sure but haven't had a chance to really look yet. I believe they just released poetry v2 so it's quite possible there were some API changes. If we are doing a blind pip install poetry without specifying the max version we may be getting the wrong one.
I will take a look later today when I get a chance.
@mattkram Any idea on why the tests are failing ? - quickly looking on the internet it seems an issue with poetry version? but not sure.. REF
Tried to update
poetry-coredep to see if that fixes the issue (as per instructions from poetry#4983I'm not sure but haven't had a chance to really look yet. I believe they just released poetry v2 so it's quite possible there were some API changes. If we are doing a blind
pip install poetrywithout specifying the max version we may be getting the wrong one.I will take a look later today when I get a chance.
So what I did was essentially changing (manually) the requirement for poetry-core >=1.1.0b3. The last comment in the issue is indeed dated on 15th of July so the problem (and workaround) seems to be there still.
However, looking at setup in github actions, poetry 1.1.6 seems to be installed, instead of poetry 1.2 - not sure how I could control this? π€
Codecov Report
Merging #36 (29bfdda) into main (4477ce6) will decrease coverage by
2.72%. The diff coverage is91.08%.
@@ Coverage Diff @@
## main #36 +/- ##
==========================================
- Coverage 96.78% 94.06% -2.73%
==========================================
Files 10 12 +2
Lines 249 438 +189
==========================================
+ Hits 241 412 +171
- Misses 8 26 +18
| Impacted Files | Coverage Ξ | |
|---|---|---|
| src/pyscript/_generator.py | 82.92% <70.83%> (-17.08%) |
:arrow_down: |
| src/pyscript/plugins/wrap.py | 97.82% <88.88%> (-2.18%) |
:arrow_down: |
| src/pyscript/_node_parser.py | 90.19% <90.19%> (ΓΈ) |
|
| src/pyscript/_supported_packages.py | 100.00% <100.00%> (ΓΈ) |
|
| src/pyscript/cli.py | 88.57% <100.00%> (+1.47%) |
:arrow_up: |
| tests/test_cli.py | 100.00% <100.00%> (ΓΈ) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@leriomaggio I believe the poetry issue was that there was an added section in pyproject.toml that was invalid. I commented it out, but we should remove it before we merge. Just wanna check that they are valid actual dependencies, or if they should be moved to the dev dependencies section.
I also cleaned up some type hints.
I will give the rest of the code a more thorough review later, but I wanted to get the CI passing for you. Gotta hone this automation (or throw it away if it is too onerous for contributors).
@mattkram thanks very much for taking the time to have a look at that - and for making the automation to pass.
Re poetry and group: I thought the idea was to keep deps separated from installation (from source) and dev/contribution.
In fact, in the new section "how to setup the env" I added in CONTRIBUTING.md, I mentioned the two options.
I think the question is whether we want to have these groups, or to keep it simple? Or maybe move that one to extra too?
So far the dev group would merely include pre-commit, to setup hooks that would help prepping the code for PR and adhere to coding conventions, and pytest. On this note, thanks for fixing the typing annotation I missed! π Appreciated!
As for the other deps in poetry: I think I double checked those, so they should all be required by pyscript-cli to run - including the new nbconvert one for notebook conversion. But please let me know, in case :)
FWIW, in the plugins we're developing (with poetry) we're using the new groups based dependency schema as per poetry 1.2.
Thanks @ntoll for the review and for finding the time to go though the code! π
Now comes the interesting bit,
[...]This is all very very interesting! I am very much looking forward to knowing more about this!
While I've approved this PR, FIRST we need to decide (all input welcome!): [...]
This is indeed a very good point! Just throwing a couple of points here that could hopefully give food for thoughts to help deciding about this (or not?! π )
-
If I understood correctly, what's going to be soon deprecated is how the
py-envenvironment is defined. Which means, for the sake of this PR, changing how the dependencies will finally end up in the template. (pls correct me if I am wrong!). So, from my perspective, this would mean fixing only half of it... :) -
I am working on my upcoming Webinar on PyScript for Nucleus (due on the 21st) and I was hoping to mention
pyscript-cliand have it showcased as well! I will specifically mentionpy-envas a key PyScript's feature for Data Science, so havingcliwithpy-envsupport would've been super great (from my very biased pov)
HTH
@leriomaggio Seems like this PR was forgotten for a long time π¬ I'm going to try and fix the conflicts and make the necessary changes to get this in π