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

MAINT: bump deps and run copier with py3.10 #507

Merged
merged 18 commits into from
May 15, 2024
Merged

Conversation

MridulS
Copy link
Member

@MridulS MridulS commented Apr 23, 2024

No description provided.

@YooSunYoung
Copy link
Member

@scipp/ess-maintainers Here as well. Can someone please update the protection rules?

Copy link
Member

@nvaytet nvaytet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI didn't run because of an invalid yml file:

Invalid workflow file: .github/workflows/ci.yml#L73
The workflow is not valid.
In .github/workflows/ci.yml (Line: 73, Col: 11):
Error from called workflow 26b3c1e (Line: 62, Col: 9): Unexpected value 'with'

@MridulS MridulS enabled auto-merge April 25, 2024 11:29
@MridulS
Copy link
Member Author

MridulS commented Apr 29, 2024

Trying to run tests with the latest releases and the mantid ones are failing on scipp related ones. Need to investigate this.

@jl-wynen
Copy link
Member

jl-wynen commented May 3, 2024

@nvaytet Can you check if your comment was addressed? The PR isn't merging because you requested a change.

- scipp>=23.07.0
- scippnexus>=23.08.0
- scipy>=1.7.0
- python>=3.10

{% for package in dependencies %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now duplicating the dependencies that are listed above. We should remove the ones above, and only keep python>=3.10.

@@ -3,16 +3,13 @@
import doctest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check that the rendered docs still look as they should?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not. I can not build docs locally as mantid doesn't provide conda packages for arm based Macs.

Looking at the doc build log (which has a green check) it is failing all around inside the doc build! I will look into it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually just download the build docs artifact from the Checks tab, unzip and view locally in a browser

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The artifacts are empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that this did not fail the build! 🤔
The error

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In[6], line 1
----> 1 scn.instrument_view(detector.sum('tof'),norm='log')

File ~/work/scippneutron/scippneutron/src/scippneutron/instrument_view.py:277, in instrument_view(scipp_obj, positions, pixel_size, components, **kwargs)
    2[74](https://github.com/scipp/scippneutron/actions/runs/8966725725/job/24623041826#step:7:75)         pixel_size = np.linalg.norm(pos_array[1] - pos_array[0])
    2[76](https://github.com/scipp/scippneutron/actions/runs/8966725725/job/24623041826#step:7:77) fig = pp.scatter3d(scipp_obj, pos=positions, pixel_size=pixel_size, **kwargs)
--> 2[77](https://github.com/scipp/scippneutron/actions/runs/8966725725/job/24623041826#step:7:78) scene = fig.canvas.scene
    279 # Add additional components from the beamline
    280 if components:

AttributeError: 'Box' object has no attribute 'canvas'

is because we need to update plopp in the .buildconfig/ci-linux.yml file to 24.04

prompt-toolkit==3.0.36
# Temporary pinned until prompt-tookit conflict is resolved.
ipython==8.9.0
scipp[all]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that scipp[all] was removed. Should it be there or should it not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to say that yes it should be there? But why none of the actions are tripping up after removing scipp from base requirements 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that scipp is still there in the requirements further down (just without the [all])

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I guess then we shouldn't add the all optional identifier. It was added in #471

python -m sphinx -j2 -v -b html -d doctrees docs html
python -m sphinx -j2 -v -b doctest -d doctrees docs html
python -m sphinx -W -j2 -v -b html -d doctrees docs html
python -m sphinx -W -j2 -v -b doctest -d doctrees docs html
find html -type f -name "*.ipynb" -not -path "html/_sources/*" -delete
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the docs builds not being reported as failing because this line runs after the docs build and succeeds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but for now I will create a new issue to track this so this PR isn't blocked. I will fix the issues in the notebooks manually.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #510 to track this.

@MridulS
Copy link
Member Author

MridulS commented May 7, 2024

I think this is good to go @nvaytet , and running the sphinx build commands independently may have fixed #510 too

@MridulS MridulS requested a review from nvaytet May 7, 2024 08:47
@MridulS MridulS merged commit 8c28b86 into scipp:main May 15, 2024
4 checks passed
@MridulS MridulS deleted the copier_update branch May 15, 2024 12:08
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.

4 participants