Skip to content
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

Add manifest ci & update manifest #130

Merged
merged 9 commits into from
Jan 21, 2025
Merged

Add manifest ci & update manifest #130

merged 9 commits into from
Jan 21, 2025

Conversation

jluethi
Copy link
Contributor

@jluethi jluethi commented Jan 16, 2025

@nrepina it looks like your way of recreating the manifest leads to some potential issues where extra spaces get added. Maybe a IDE setting where you normalize some files?

We test that some core packages like scmultiplex have their manifest created correctly in fractal tasks core. scmultiplex raised some issues now. I fixed those with this PR. And I suggest we add a check to the CI that the manifest as it is committed is what the CI and other ways of building it would create as well.

Happy to help spot what causes the issue on your end when I'm in Basel next :)

@jluethi
Copy link
Contributor Author

jluethi commented Jan 17, 2025

@tcompa Could you help out here with the way we check for whether the manifest is correct?

There appears to be some issue with our checking action here. By default, I run into the following error:

Traceback (most recent call last):
  File "/home/runner/work/gliberal-scMultipleX/gliberal-scMultipleX/src/scmultiplex/dev/create_manifest.py", line 10, in <module>
    create_manifest(package=PACKAGE, authors=AUTHORS, docs_link=docs_link)
  File "/opt/hostedtoolcache/Python/3.9.21/x64/lib/python3.9/site-packages/fractal_tasks_core/dev/create_manifest.py", line 91, in create_manifest
    task_list_module = import_module(f"{package}.dev.task_list")
  File "/opt/hostedtoolcache/Python/3.9.21/x64/lib/python3.9/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
  File "<frozen importlib._bootstrap>", line 972, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line [22](https://github.com/fmi-basel/gliberal-scMultipleX/actions/runs/12814665214/job/35731612906#step:6:23)8, in _call_with_frames_removed
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
  File "<frozen importlib._bootstrap>", line 984, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'scmultiplex.dev'

If I add an init.py file to the dev folder (which isn't necessary in other packages), I hit:

Traceback (most recent call last):
  File "/home/runner/work/gliberal-scMultipleX/gliberal-scMultipleX/src/scmultiplex/dev/create_manifest.py", line 10, in <module>
    create_manifest(package=PACKAGE, authors=AUTHORS, docs_link=docs_link)
  File "/opt/hostedtoolcache/Python/3.9.21/x64/lib/python3.9/site-packages/fractal_tasks_core/dev/create_manifest.py", line 138, in create_manifest
    docs_info = read_docs_info_from_file(
  File "/opt/hostedtoolcache/Python/3.9.21/x[64](https://github.com/fmi-basel/gliberal-scMultipleX/actions/runs/12833009036/job/35787024646#step:6:65)/lib/python3.9/site-packages/fractal_tasks_core/dev/lib_task_docs.py", line 114, in read_docs_info_from_file
    with open(docs_path, "r") as f:
FileNotFoundError: [Errno 2] No such file or directory: '/opt/hostedtoolcache/Python/3.9.21/x64/lib/python3.9/site-packages/scmultiplex/dev/task_info/calculate_object_linking.md'

If I add init.py to both folders, I still hit the error from above. Any idea what is configured wrong here? Are there assumptions in our create_manifest that don't hold for those setup.cfg configured Python projects?

@tcompa
Copy link
Collaborator

tcompa commented Jan 21, 2025

For the record, this does not take place when trying to reproduce it locally

  • git clone repo, and switch to add_manifest_CI branch
  • create venv, and pip install -e .[extras]
  • run create_manifest.py -> no errors

I'll have a look at the GitHub action

@jluethi
Copy link
Contributor Author

jluethi commented Jan 21, 2025

Thanks @tcompa ! Yes, it also works fine for me locally

@tcompa
Copy link
Collaborator

tcompa commented Jan 21, 2025

@jluethi I cannot push to this branch. Could you try to replace

python -m pip install .[test,fractal-tasks,spherical-harmonics]

with

python -m pip install -e .[test,fractal-tasks,spherical-harmonics]

in the GitHub action yaml file?

The explanation is as follows:
In the internal manifest-creation tools, we import the task-list Python module directly from the actual Python package

    # Import the task list from `dev/task_list.py`
    task_list_module = import_module(f"{package}.dev.task_list")
    TASK_LIST = getattr(task_list_module, "TASK_LIST")

and then this works with/without -e:

  1. In editable mode (that is, with -e), the source for that module is e.g. in src/scmultiplex/dev/task_list.py.
  2. In non-editable mode, the source for that module is e.g. in /opt/hostedtoolcache/Python/3.10.16/x64/lib/python3.10/site-packages/scmultiplex/dev/task_list.py

Later during manifest-creation, we look for the task_info folder in the same parent folder of task_list.py. This does exist, if we are in case 1 (editable mode), but it does not exist if we are in case 2 (since the default is that the built Python package would not include markdown files). I think this explains the FileNotFoundError.

If we don't want to depend on the fact that we are in an editable install of the relevant package for which we want to build the manifest, then we need to make an assumption about where the task_info folder is. I don't think it's worth it to add it to the built package.

@jluethi
Copy link
Contributor Author

jluethi commented Jan 21, 2025

Thanks @tcompa , that does the trick & and thanks for the explanation! Makes sense.

@nrepina I'll merge to make our CIs pass again and to have this test included in future PRs. Let's review together next week if we can figure out what causes the formatting issue of the manifest.

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.55%. Comparing base (4c8ebe3) to head (04ee68e).
Report is 33 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##            main     #130       +/-   ##
==========================================
+ Coverage   0.26%   50.55%   +50.29%     
==========================================
  Files         55       55               
  Lines       4942     4943        +1     
==========================================
+ Hits          13     2499     +2486     
+ Misses      4929     2444     -2485     

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

@jluethi jluethi merged commit ee9cbb0 into main Jan 21, 2025
5 checks passed
@jluethi jluethi deleted the add_manifest_CI branch January 21, 2025 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants