Skip to content

Cap islpy below 2026.2 on main - #5377

Closed
pbrubeck wants to merge 1 commit into
mainfrom
pbrubeck/loopy-main
Closed

pbrubeck wants to merge 1 commit into
mainfrom
pbrubeck/loopy-main

Conversation

@pbrubeck

@pbrubeck pbrubeck commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

islpy 2026.2 removed the implicit conversion of a BasicSet to a Set. loopy 2025.2 — the newest release, and what loopy>2024.1 resolves to — calls BasicSet.make_disjoint() in count_insn_runs, so every flop count in the test suite fails:

AttributeError: 'islpy._isl.BasicSet' object has no attribute 'make_disjoint'

loopy converts explicitly on main, but taking loopy from git is not an option yet: its _AccessCheckMapper.map_subscript asks for the dependencies of every shape axis without guarding None, and PyOP2 builds args with an unknown leading axis (dat0 has shape=(None, 2) in wrap_to_reference_coords), so firedrake-check dies with DependencyMapperWithReductionInames encountered invalid foreign object: None.

So cap islpy at the last version that still converts. With islpy 2026.1 and loopy 2025.2, tests/tsfc and tests/firedrake/regression/test_locate_cell.py are green locally.

Lift the cap once loopy makes a release containing the explicit to_set(). #5378 does the same for release.


AI was used in preparing this contribution (Claude Code).

🤖 Generated with Claude Code

connorjward
connorjward previously approved these changes Aug 24, 2026

@connorjward connorjward 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.

Can there be an equivalent PR into release that pins islpy?

@pbrubeck

Copy link
Copy Markdown
Contributor Author

Can there be an equivalent PR into release that pins islpy?

Sure, I'll open it.

islpy 2026.2 removed the implicit conversion of a BasicSet to a Set, so
loopy 2025.2 raises AttributeError from count_insn_runs when it reaches
BasicSet.make_disjoint, and every flop count in the test suite fails.

loopy has released nothing since, so hold islpy at the last version that
still converts. Lift the cap once loopy releases the fix.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pbrubeck
pbrubeck force-pushed the pbrubeck/loopy-main branch from a56d0cd to 5c03143 Compare August 24, 2026 09:35
@pbrubeck pbrubeck changed the title Install loopy from main Cap islpy below 2026.2 Aug 24, 2026
@pbrubeck

Copy link
Copy Markdown
Contributor Author

@connorjward It seems that the right fix is to cap islpy on main too. Do we want two separate PRs?

@pbrubeck pbrubeck changed the title Cap islpy below 2026.2 Cap islpy below 2026.2 on main Aug 24, 2026
@connorjward

Copy link
Copy Markdown
Contributor

@connorjward It seems that the right fix is to cap islpy on main too. Do we want two separate PRs?

Why is it needed on main? I would prefer on main to follow upstream branches.

@francesco-ballarin

Copy link
Copy Markdown
Member

While this might be a good short-term solution, can you work with upstream islpy/loopy to fix the issue?

I had to open a PR on islpy (which then generated a new release 2026.2.1) to fix compatibility with nanobind 3.0, and if islpy gets pinned it won't have that fix.
(I can patch my way out of it, sure, but only for the short term)

@pbrubeck

pbrubeck commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Why is it needed on main? I would prefer on main to follow upstream branches.

We cannot pin islpy and loopy to main. There is a chain of breakages:

Break 1 — loopy/statistics.py:1454 (loopy 2025.2) does set = set.make_disjoint() where set may be a BasicSet; that only worked via islpy's auto-promotion to Set, removed in islpy 2026.2.

Break 2 — pyop2/codegen/builder.py: shape = (None, arity) at line 50 (map arguments) and shape = (None, *arg.dim) at lines 861/879 (dat arguments), with Argument((None, 2), IntType, name="layers") at line 759. Those flow into pyop2/codegen/rep2loopy.py:797, which builds loopy.GlobalArg(name, dtype=dtype, shape=shape, strides=loopy.auto). loopy main then walks that shape in _AccessCheckMapper.map_subscript at loopy/check.py:771 — shape_deps.update(get_dependencies(shape_axis)) — and pymbolic raises on the None axis. check.py:1629 already guards this with if shape_axis is not None, and loopy/kernel/array.py:590 handles None axes too, so 771 is the only unguarded site. Adding the guard there is necessary but not sufficient: with it, tests/tsfc passes (369) but test_locate_cell still fails, so at least one further incompatibility sits behind it.

@connorjward

Copy link
Copy Markdown
Contributor

Why is it needed on main? I would prefer on main to follow upstream branches.

We cannot pin islpy and loopy to main. There is a chain of breakages:

Break 1 — loopy/statistics.py:1454 (loopy 2025.2) does set = set.make_disjoint() where set may be a BasicSet; that only worked via islpy's auto-promotion to Set, removed in islpy 2026.2.
Break 2 — pyop2/codegen/builder.py: shape = (None, arity) at line 50 (map arguments) and shape = (None, *arg.dim) at lines 861/879 (dat arguments), with Argument((None, 2), IntType, name="layers") at line 759. Those flow into pyop2/codegen/rep2loopy.py:797, which builds loopy.GlobalArg(name, dtype=dtype, shape=shape, strides=loopy.auto). loopy main then walks that shape in _AccessCheckMapper.map_subscript at loopy/check.py:771 — shape_deps.update(get_dependencies(shape_axis)) — and pymbolic raises on the None axis. check.py:1629 already guards this with if shape_axis is not None, and loopy/kernel/array.py:590 handles None axes too, so 771 is the only unguarded site. Adding the guard there is necessary but not sufficient: with it, tests/tsfc passes (369) but test_locate_cell still fails, so at least one further incompatibility sits behind it.

I don't understand break 2

@connorjward

Copy link
Copy Markdown
Contributor

I think pinning islpy is a non-starter. We have to address this more fundamentally. If loopy main gets things to pass then we should pursue getting a new loopy release made. If it doesn't pass then we should fix whatever fails.

@connorjward

Copy link
Copy Markdown
Contributor

While this might be a good short-term solution, can you work with upstream islpy/loopy to fix the issue?

I had to open a PR on islpy (which then generated a new release 2026.2.1) to fix compatibility with nanobind 3.0, and if islpy gets pinned it won't have that fix. (I can patch my way out of it, sure, but only for the short term)

I've opened a loopy issue inducer/loopy#1039. Contrary to what I said before I think we do have to pin islpy because I don't know the loopy fix and we need CI back up.

@connorjward

Copy link
Copy Markdown
Contributor

Closing this one as #5378 has just been merged into release and will be carried over ASAP.

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.

3 participants