pyscript-cli icon indicating copy to clipboard operation
pyscript-cli copied to clipboard

Support for Automatic <py-env> with dependency + few tweaks to contributing and readme docs

Open leriomaggio opened this issue 3 years ago β€’ 12 comments

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 paths as 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.
  • 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.md documentation providing instructions on how to setup dev environment (not just for docs)
    • on this note, poetry dependencies have been revised and updated, including pre-commit as dependency, with instructions in the main doc on how to install hooks.
  • added line in README.md to showcase input code from both python and notebooks files.

leriomaggio avatar Sep 07 '22 15:09 leriomaggio

@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"

mattkram avatar Sep 07 '22 16:09 mattkram

@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"

YES! that was giving me headaches already!! will do :D

leriomaggio avatar Sep 07 '22 16:09 leriomaggio

@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

leriomaggio avatar Sep 07 '22 16:09 leriomaggio

@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

leriomaggio avatar Sep 07 '22 16:09 leriomaggio

@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

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 avatar Sep 07 '22 17:09 mattkram

@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

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.

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? πŸ€”

leriomaggio avatar Sep 07 '22 17:09 leriomaggio

Codecov Report

Merging #36 (29bfdda) into main (4477ce6) will decrease coverage by 2.72%. The diff coverage is 91.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.

codecov-commenter avatar Sep 07 '22 17:09 codecov-commenter

@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 avatar Sep 07 '22 17:09 mattkram

@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 :)

leriomaggio avatar Sep 08 '22 09:09 leriomaggio

FWIW, in the plugins we're developing (with poetry) we're using the new groups based dependency schema as per poetry 1.2.

ntoll avatar Sep 08 '22 12:09 ntoll

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?! πŸ˜… )

  1. If I understood correctly, what's going to be soon deprecated is how the py-env environment 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... :)

  2. I am working on my upcoming Webinar on PyScript for Nucleus (due on the 21st) and I was hoping to mention pyscript-cli and have it showcased as well! I will specifically mention py-env as a key PyScript's feature for Data Science, so having cli with py-env support would've been super great (from my very biased pov)

HTH

leriomaggio avatar Sep 08 '22 13:09 leriomaggio

@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 πŸ‘

FabioRosado avatar Feb 26 '24 17:02 FabioRosado