Revert "Also use poetry for calling jupytext"

[hugobuddel]

This reverts commit b3cafca.

That is, it moves the poetry run from insdie runnotebooks.sh to notebooks_dispatch.yml that runs runnotebooks.sh.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d762bcf) 77.04% compared to head (547e08f) 77.04%.

Additional details and impacted files

@@             Coverage Diff             @@
##           dev_master     #334   +/-   ##
===========================================
  Coverage       77.04%   77.04%           
===========================================
  Files              57       57           
  Lines            7705     7705           
===========================================
  Hits             5936     5936           
  Misses           1769     1769           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[teutoburg]

Jössas, I need to disable this github actions bot, it's spamming like crazy...

[hugobuddel]

So this does indeed not work, because jupytext is not in the path. On my local machine it isn't either, but I don't understand why.

Both jupyter and jupytext are listed as dev dependency in pyproject.toml:

[tool.poetry.group.dev.dependencies]
jupyter = "^1.0"
jupytext = "^1.10.0"
ipykernel = "^6.24.0"

but for some reason jupytext is not installed when doing poetry install --all-extras.

[hugobuddel]

Hmm I think I don't understand how poetry interact with pip.

Fetching ScopeSim from PyPI does install the dev dependencies:

pip install ScopeSim[test,dev,docs]

But installing from a local checkout does not:

pip install .[test,dev,docs]

I guess this makes sense, because poetry has not yet converted the 'poetry' dependencies into 'true' dependencies.

ScopeSim_Data needs to install ScopeSim from source, because the idea is that it fetches any new data before a ScopeSim release is made, so it indeed must use poetry as well.

However, I expect that poetry run would not be necessary, as it seems that jupytext will be installed in the path, if ScopeSim is installed with poetry.

[teutoburg]

but for some reason jupytext is not installed when doing poetry install --all-extras.

Because it's not an extra but an optional dependency group. poetry install --with dev should do that. Poetry is a bit weird in that regard, but now that I understood what's what, I do think it's actually a good thing to split that.

[hugobuddel]

but for some reason jupytext is not installed when doing poetry install --all-extras.

Because it's not an extra but an optional dependency group. poetry install --with dev should do that. Poetry is a bit weird in that regard, but now that I understood what's what, I do think it's actually a good thing to split that.

OK, I get the difference. But now I don't understand why it cannot find jupytext. notebooks_dispatch.yml does

poetry install --with test --with dev --all-extras

and if I run that as well, then I get jupytext installed in the path, next to the python executable etc., so it is not necessary to use poetry run.

Perhaps there is a difference in how the environment is set up, but I don't understand what. The github action we use, simply does

pip install poetry==${{ inputs.poetry-version }}

where we set the version to 1.7.1. I did pip install poetry locally and also got 1.7.1.

So I don't know what the difference is between the environment on my machine and on the github action that makes it necessary to use poetry run.

[hugobuddel]

So I don't know what the difference is between the environment on my machine and on the github action that makes it necessary to use poetry run.

On github:

Run poetry install --with test --with dev --all-extras
[...]
Creating virtualenv scopesim-KL_B9KV7-py3.11 in /home/runner/.cache/pypoetry/virtualenvs
Installing dependencies from lock file
[...]

On my machine:

$ poetry install --with test --with dev --all-extras
Installing dependencies from lock file
[...]

So the "Creating virtualenv" is something that does not happen locally, but I haven't yet figured out why it does happen in the github action.

If we can prevent the github action from creating a virtualenv, then it should not be necessary to have poetry run in runnotebooks.sh, making it trivial to run for all users.

[teutoburg]

So the "Creating virtualenv" is something that does not happen locally, but I haven't yet figured out why it does happen in the github action.

Having used Poetry for several projects now: It does create a virtual env locally on my machine, the first time I run poetry install for any given project. Or after I delete the env and run it again. Not sure if OS makes a difference here, for what it's worth, I'm on Win11.

[teutoburg]

When I run poetry install --with dev and then try to just run jupytext, it fails (does not recognize the command, i.e. not installed). When I try poetry run jupytext instead, it works. Which is, as far as I can tell, the same behavior we get on github.

[hugobuddel]

So the "Creating virtualenv" is something that does not happen locally, but I haven't yet figured out why it does happen in the github action.

Having used Poetry for several projects now: It does create a virtual env locally on my machine, the first time I run poetry install for any given project. Or after I delete the env and run it again. Not sure if OS makes a difference here, for what it's worth, I'm on Win11.

Thanks for the verification. It probably does not happen on my machine because I always create a new virtual environment with conda, and from the documentation:

External virtual environment management

Poetry will detect and respect an existing virtual environment that has been externally activated. This is a powerful mechanism that is intended to be an alternative to Poetry’s built-in, simplified environment management.

To take advantage of this, simply activate a virtual environment using your preferred method or tooling, before running any Poetry commands that expect to manipulate an environment.

However, I have never ran poetry install outside a virtual environment, since I didn't know this respect for existing virtual environments, and it seemed (to me) that poetry would just install packages all over the environment it is in (without asking confirmation), and polluting my base environment is something I'd like to prevent at all costs...

So if we want end users to be able to run runnotebooks.sh, then we are at a conundrum, because we don't want to require them to install poetry, yet poetry is necessary to get jupytext, because jupytext is in a 'group' instead of in an 'extra'.

Perhaps it would be better to hide runnotebooks.sh somewhere and add instructions that poetry is necessary to run it.

As for the immediate problem, I believe that AstarVienna/ScopeSim_Data#10 will fix the problem in ScopeSim_Data, but the latest attempt failed with "ContentTooShortError".

[teutoburg]

Perhaps it would be better to hide runnotebooks.sh somewhere and add instructions that poetry is necessary to run it.

Or get rid of it all together and use pytest_notebook instead? 🤓

Is there any reason we (or someone else) might need to run runnotebooks.sh (or any equivalent thereof) outside of testing? I could also imagine a gh actions workflow file with that script...

[hugobuddel]

Perhaps it would be better to hide runnotebooks.sh somewhere and add instructions that poetry is necessary to run it.

Or get rid of it all together and use pytest_notebook instead? 🤓

Is there any reason we (or someone else) might need to run runnotebooks.sh (or any equivalent thereof) outside of testing? I could also imagine a gh actions workflow file with that script...

runnotebooks.sh is indeed mainly for testing. But my stupid head put it in the root of the repository, so it is easily discoverable, and then it should work in my opinion. My suggestion to move it could be seen as an interim solution.

If pytest_notebook is an easy replacement, then we should just use that.

But there is also this cruft in runnotebooks.sh that makes the notebooks faster to run so they actually run as a github action.

[teutoburg]

If pytest_notebook is an easy replacement, then we should just use that.

Not necessarily easy. But maybe still worth it. I'd like to look into it more, but maybe not right now (too busy with other things tbh).

[hugobuddel]

This should work, because the notebook test runs.

The Windows 3.12 test fails due to AstarVienna/DevOps#12 , which is unrelated to this change.