Skip to content

Move imports for should-be-optional dependencies#1606

Merged
erogluorhan merged 3 commits into
mainfrom
move_imports_for_should-be-optional_deps
Jul 24, 2026
Merged

Move imports for should-be-optional dependencies#1606
erogluorhan merged 3 commits into
mainfrom
move_imports_for_should-be-optional_deps

Conversation

@Sevans711

Copy link
Copy Markdown
Collaborator

One step towards solving #1224, but not the full solution.

Overview

Moves imports for "should-be-optional" dependencies to be inside functions instead of at tops of files. All of these changes were originally proposed in PR #1548, which also includes detailed discussion about which dependencies should be optional. However, the changes here do not include edits to pyproject.toml; the should-be-optional dependencies remain fully required, leaving installation processes fully untouched.

The narrower scope of this PR aims to provide simpler-to-accept changes. It was created in response to concerns about the size of PR 1548.

Recommended action plan:

  1. Review this PR. Discuss as needed.
  2. When approved, merge to main.
  3. Merge main into the original PR (1548). This will reduce the number of changes in PR 1548 significantly.
  4. PR 1548 will continue to be responsible for changes to pyproject.toml, optional dependencies testing, etc. Or, maybe some of those details will be split into other PRs, too; that can be discussed in thread on PR 1548 directly.

PR Checklist

General

  • An issue is linked created and linked
  • Add appropriate labels
  • Filled out Overview and Expected Usage (if applicable) sections

such imports are now inside functions instead of at top level. This will support making them optional later in pyproject.toml. This splits off of #1548 to reduce scope of that PR; see that PR for more details.
@Sevans711 Sevans711 added the dependencies Pull requests that update a dependency file label Jul 22, 2026
@Sevans711 Sevans711 changed the title Move imports for should-be-optional deps Move imports for should-be-optional dependencies Jul 22, 2026
@Sevans711 Sevans711 mentioned this pull request Jul 22, 2026
3 tasks
@Sevans711
Sevans711 requested a review from rajeeja July 22, 2026 21:21

@rajeeja rajeeja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed and tested locally — checked out the branch, ran the full suite (565 passed) plus a pre-commit pass, and traced every moved import (ccrs/GeoAxes in dataarray.py/grid.py, pyproj.Geod in sample.py, antimeridian/shapely in geometry.py, healpix in _healpix.py, holoviews in plot/utils.py, scipy.sparse in remap/weights.py) to confirm no runtime usage was left behind outside the function scope it moved into. Clean, well-scoped split from #1548. Approving.

@erogluorhan

Copy link
Copy Markdown
Member

... traced every moved import (ccrs/GeoAxes in dataarray.py/grid.py, pyproj.Geod in sample.py, antimeridian/shapely in geometry.py, healpix in _healpix.py, holoviews in plot/utils.py, scipy.sparse in remap/weights.py) to confirm no runtime usage was left behind outside the function scope it moved into.

I was wondering if confirmed we had test coverage for all these imports, so I can ask that to you: Did you trace them yourself by eye, or did you confirm test coverage as well? If the latter, I think we can go ahead and merge this; otherwise, I can dod that check.

@cmdupuis3

Copy link
Copy Markdown
Collaborator

I'll trace the dependencies and close this out if I find it's working well.

@erogluorhan erogluorhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great to me! Once I hear back to my question above, I'll approve this.

@cmdupuis3

Copy link
Copy Markdown
Collaborator

Yeah I don't see anything missing from the stack traces.

@erogluorhan

Copy link
Copy Markdown
Member

Yeah I don't see anything missing from the stack traces.

Thanks very much for looking into this, Chris!

@erogluorhan
erogluorhan self-requested a review July 24, 2026 17:44

@erogluorhan erogluorhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is great; thanks Sam!

@erogluorhan
erogluorhan merged commit ac309b0 into main Jul 24, 2026
14 checks passed
@Sevans711
Sevans711 deleted the move_imports_for_should-be-optional_deps branch July 24, 2026 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants