core: Fix check for forward refs by checking for ssa refs instead of block refs #3851

emmau678 · 2025-02-06T10:14:25Z

Change check for forward block refs to forward SSA refs for correctness of the plural case

Can be replaced with `MemRefType.constr()`

This PR: - Disallows duplicate keys in properties and attribute dictionaries - Tests of the above Bumped into this by accident and the tests are similar to https://github.com/llvm/llvm-project/blob/46b1543dc04970719caab0d4f9f65699fea6adbc/mlir/test/IR/invalid-builtin-attributes.mlir#L485 I don't venture often in the parser, so any feedback is welcome.

Bumps [marimo](https://github.com/marimo-team/marimo) from 0.10.19 to 0.11.0. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/marimo-team/marimo/releases">marimo's releases</a>.</em></p> <blockquote> <h2>0.11.0</h2> <h2>Highlights ⭐</h2> <ol> <li><strong>SQL Engine Support</strong>. Connect to various databases like postgresql, mysql, snowflake and more, using your preferred SQL engine.</li> </ol> <p>This release adds support for using for multiple SQL connection libraries, such as SQLModel and SQLAlchemy. You can now define SQL connections in your code like:</p> <pre lang="python"><code>import sqlalchemy import sqlmodel import duckdb <h1>Create an in-memory SQLite database with SQLAlchemy</h1> <p>sqlite_engine = sqlachemy.create_engine("sqlite:///:memory:")</p> <h1>Create a Postgres database with SQLModel</h1> <p>postgres_engine = sqlmodel.create_engine("postgresql://username:password@server:port/database")</p> <h1>Create a DuckDB connection</h1> <p>duckdb_conn = duckdb.connect("file.db") </code></pre></p> <p>And then select which connection to use in the SQL cell.</p> <p><img src="https://github.com/user-attachments/assets/8ba7f094-aefd-4cfc-95c0-19da5ee3378e" alt="image" /></p> <p>h/t <a href="https://github.com/Light2Dark"><code>@Light2Dark</code></a></p> <ol start="2"> <li><strong>Markdown file-format improvements</strong> - Markdown notebooks (i.e. <code>marimo edit notebook.md</code>) has an improved syntax format: <code>python {.marimo}</code>. You can also use SQL cells in the markdown file-format, using <code>sql {.marimo}</code>. To learn more, run <code>marimo tutorial markdown-format</code></li> </ol> <p>h/t <a href="https://github.com/dmadisetti"><code>@dmadisetti</code></a></p> <ol start="4"> <li> <p><strong>Markdown syntax</strong> - Added support for details, admonitions, and emojis in markdown</p> </li> <li> <p><strong>Performance & Reliability</strong> - Lots of bug fixes for better resource cleanup and memory management, as well as disabling features not used in run-mode.</p> </li> </ol> <h2>What's Changed</h2> <ul> <li>feat: SQL engines by <a href="https://github.com/mscolnick"><code>@mscolnick</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3563">marimo-team/marimo#3563</a></li> <li>update: update code block to ```python {.marimo} by <a href="https://github.com/dmadisetti"><code>@dmadisetti</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3387">marimo-team/marimo#3387</a></li> <li>chore: add hatch build hook to build marimo when installed from github by <a href="https://github.com/mscolnick"><code>@mscolnick</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3639">marimo-team/marimo#3639</a></li> <li>docs: typo by <a href="https://github.com/mscolnick"><code>@mscolnick</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3641">marimo-team/marimo#3641</a></li> <li>tests: add tests for try-format by <a href="https://github.com/mscolnick"><code>@mscolnick</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3643">marimo-team/marimo#3643</a></li> <li>fix: duckdb querying dataframes with engines by <a href="https://github.com/mscolnick"><code>@mscolnick</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3642">marimo-team/marimo#3642</a></li> <li>fix: show 'Clear Output' cell action when console has output by <a href="https://github.com/Light2Dark"><code>@Light2Dark</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3650">marimo-team/marimo#3650</a></li> <li>fix(deps): update all npm non-major dependencies by <a href="https://github.com/renovate"><code>@renovate</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3655">marimo-team/marimo#3655</a></li> <li>improve: run mode performance for streams by <a href="https://github.com/akshayka"><code>@akshayka</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3648">marimo-team/marimo#3648</a></li> <li>chore(deps): update dependency scikit-bio to >=0.6.3 by <a href="https://github.com/renovate"><code>@renovate</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3647">marimo-team/marimo#3647</a></li> <li>chore(deps): update dependency google-generativeai to v0.8.4 by <a href="https://github.com/renovate"><code>@renovate</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3646">marimo-team/marimo#3646</a></li> <li>fix: correctness of formatting by <a href="https://github.com/mscolnick"><code>@mscolnick</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3644">marimo-team/marimo#3644</a></li> <li>improvement: removed border and and fix tooltips when multi-column view by <a href="https://github.com/Light2Dark"><code>@Light2Dark</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3649">marimo-team/marimo#3649</a></li> <li>fix: date range not defaulting to <code>start</code> and <code>stop</code> by <a href="https://github.com/Hofer-Julian"><code>@Hofer-Julian</code></a> in <a href="https://redirect.github.com/marimo-team/marimo/pull/3651">marimo-team/marimo#3651</a></li> </ul>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/marimo-team/marimo/commit/3b84b1fda1f7af570c931bf47ee990ea7cb733d9"><code>3b84b1f</code></a> release: 0.11.0</li> <li><a href="https://github.com/marimo-team/marimo/commit/2dc721c25d6027be11e76da05d4692dabd4cff3f"><code>2dc721c</code></a> fix: sql dropdown when initialized (<a href="https://redirect.github.com/marimo-team/marimo/issues/3687">#3687</a>)</li> <li><a href="https://github.com/marimo-team/marimo/commit/33dcefb76b1354c2c48609f72384c867fc195566"><code>33dcefb</code></a> fix: checks whether latestEngineSelected is in list of engines (<a href="https://redirect.github.com/marimo-team/marimo/issues/3685">#3685</a>)</li> <li><a href="https://github.com/marimo-team/marimo/commit/79da93a2ee5fef1dd65fcb5f60c87f9cbf565054"><code>79da93a</code></a> smoke test: docstring_to_markdown (<a href="https://redirect.github.com/marimo-team/marimo/issues/3686">#3686</a>)</li> <li><a href="https://github.com/marimo-team/marimo/commit/3426a56e7b0c87591bdc252a0f131c0a74a967b7"><code>3426a56</code></a> chore(deps): update dependency vitest to v3.0.5 [security] (<a href="https://redirect.github.com/marimo-team/marimo/issues/3684">#3684</a>)</li> <li><a href="https://github.com/marimo-team/marimo/commit/2ee4419f64a27cd0821e905021c96cc98b10c74d"><code>2ee4419</code></a> Add openai-whisper to module name list (<a href="https://redirect.github.com/marimo-team/marimo/issues/3681">#3681</a>)</li> <li><a href="https://github.com/marimo-team/marimo/commit/16a1d6a90e090eb759116c3c26de2856f2c137a8"><code>16a1d6a</code></a> improv: update the sql dropdown to a better design and show disconnected engi...</li> <li><a href="https://github.com/marimo-team/marimo/commit/90e398767244cf4f3a33eb4b2b8c8cd06016e659"><code>90e3987</code></a> [pre-commit.ci] pre-commit autoupdate (<a href="https://redirect.github.com/marimo-team/marimo/issues/3678">#3678</a>)</li> <li><a href="https://github.com/marimo-team/marimo/commit/c583b6dbc57b697a94331e9e3ab7e5de22bb1a88"><code>c583b6d</code></a> fix: filtering datasets based on variables (<a href="https://redirect.github.com/marimo-team/marimo/issues/3676">#3676</a>)</li> <li><a href="https://github.com/marimo-team/marimo/commit/4c6c6d1c89b619fe7b835efbf5b60d6fd3575921"><code>4c6c6d1</code></a> improvement: show engine's tables in the datasources panel (<a href="https://redirect.github.com/marimo-team/marimo/issues/3665">#3665</a>)</li> <li>Additional commits viewable in <a href="https://github.com/marimo-team/marimo/compare/0.10.19...0.11.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=marimo&package-manager=pip&previous-version=0.10.19&new-version=0.11.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: compor <[email protected]>

Makes the spelling consistent between `MemRefType` and `UnrankedMemrefType`. Fixes #3474

This adds a hidden region to the linalg.transpose op to ensure correct generic printing Also changes permutation to a property instead of attribute. This resolves the transpose op in #2959 This has now been checked manually, and will be put in ci with #3837 (but for that 3 other ops need to be fixed, PRs incoming...)

To enable correct printing of the hidden regions in generic printing. Resolves pooling in #2959 , will be tested in #3837 The constructor of these ops had to change to comply with the NamedOpBase constructor ordering of arguments

Resolves pooling in #2959 , will be tested in #3837

This enables to correct printing of the hidden regions in generic format Resolves conv ops in #2959, will be tested with #3837

…ck (#3837) This PR adds generic printing to the mlir conversion filecheck This will check whether the issues posed in #2959 are correctly resolved, by checking if mlir correctly parses the generic output of xdsl There are still reminaing issues, solved in the following PRs: (stacked on: ) #3838, #3839, #3840, #3841

`IntegerAttr` and `IntegerAttr.constr()` are sufficient

s/AnyIntegerAttr/IntegerAttr/g

Adds a default type to `FloatAttr` and removes `AnyFloatAttr` and `AnyFloatAttrConstr`

Partial revert of 27bf821

…> 100 elements (#3846) mimicking mlir behaviour This, along with #3845 should allow for very fast printing/parsing of large dense attrs, allowing for low-overhead calling of intermediate mlir-opt passes in our pipeline.

Fix the MultipleSpanParseError to print multiple lines in the error output --------- Co-authored-by: Alex Rice <[email protected]>

review-notebook-app · 2025-02-06T10:23:12Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

emmau678 · 2025-02-06T10:25:29Z

sorry - something seems to have gone wrong with rebase, will check it now

alexarice · 2025-02-06T10:29:08Z

xdsl/parser/core.py

@@ -145,7 +145,7 @@ def parse_module(self, allow_implicit_module: bool = True) -> ModuleOp:
            value_names = ", ".join(
                "%" + name for name in self.forward_ssa_references.keys()
            )
-            if len(self.forward_block_references.keys()) > 1:
+            if len(self.forward_ssa_references.keys()) > 1:


This is purely for getting the plural correct it seems

ok thanks @alexarice, I updated the description

Is this just to purely have a plural and singular form in the error?

I don't remember the initial discussion, but wouldn't something like:

value(s) used but not defined: %1, %2

value(s) used but not defined: %1

work for both cases?

I feel this what we do in most places. I was surprised that it tries to get the plural correct here (yet gets it wrong anyway)

TBH yeah it seems like a reasonable simplifying change. Maybe even something like this:

values used but not defined: [%1, %2]
values used but not defined: [%1]

easier to grep

@emmau678 let's update to do something like this? maybe values [%1, %2] used but not defined?

alexarice · 2025-02-06T10:30:32Z

tests/filecheck/parser-printer/graph_region.mlir

+    %0 = "test.termop"(%1, %2) : (i32, i32) -> i32
+}
+
+// CHECK: values %1, %2 were used but not defined


Can the check statement go before the code? I'm also not sure the comment is necessary, the error message describes what is going on.

moved it, thanks

Unless github is being weird, it seems you moved a different one?

I think the comment can stay since we are doing it for the rest of the file.

I have a different question though; how does this differ from the test case directly above?
(maybe I need more coffee?)

alexarice · 2025-02-06T10:31:37Z

tests/filecheck/parser-printer/graph_region.mlir

@@ -73,7 +83,7 @@ builtin.module {
        "test.op"() : () -> ()
 }

-// CHECK:       /graph_region.mlir:72:4
+// CHECK:       /graph_region.mlir:82:4


This is the fun I was hoping to avoid

I still think it's worth it :)

codecov · 2025-02-06T10:40:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.28%. Comparing base (e0473a4) to head (b356946).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3851   +/-   ##
=======================================
  Coverage   91.27%   91.28%           
=======================================
  Files         461      461           
  Lines       57663    57663           
  Branches     5573     5573           
=======================================
+ Hits        52633    52635    +2     
+ Misses       3608     3607    -1     
+ Partials     1422     1421    -1

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

compor · 2025-02-06T10:38:38Z

tests/filecheck/parser-printer/graph_region.mlir

+    %0 = "test.termop"(%1, %2) : (i32, i32) -> i32
+}
+
+// CHECK: values %1, %2 were used but not defined


I think the comment can stay since we are doing it for the rest of the file.

I have a different question though; how does this differ from the test case directly above?
(maybe I need more coffee?)

compor · 2025-02-06T10:43:03Z

xdsl/parser/core.py

@@ -145,7 +145,7 @@ def parse_module(self, allow_implicit_module: bool = True) -> ModuleOp:
            value_names = ", ".join(
                "%" + name for name in self.forward_ssa_references.keys()
            )
-            if len(self.forward_block_references.keys()) > 1:
+            if len(self.forward_ssa_references.keys()) > 1:


Is this just to purely have a plural and singular form in the error?

I don't remember the initial discussion, but wouldn't something like:

value(s) used but not defined: %1, %2

value(s) used but not defined: %1

work for both cases?

tests/filecheck/parser-printer/graph_region.mlir

compor · 2025-02-06T10:45:15Z

Also, can we update the PR title? Shouldn't it be "core: Fix ..."

compor · 2025-02-06T10:47:24Z

I approved by accident!

fix: change forward block refs to forward ssa refs

e44cd87

emmau678 added the bug Something isn't working label Feb 6, 2025

emmau678 requested review from superlopuh, compor and alexarice February 6, 2025 10:14

emmau678 self-assigned this Feb 6, 2025

alexarice and others added 16 commits February 6, 2025 10:22

dialects: (builtin) remove AnyMemRefTypeConstr (#3832)

b86c47e

Can be replaced with `MemRefType.constr()`

dialects: (builtin/memref) rename Memref to MemRef everywhere (#3833)

83ee4c0

Makes the spelling consistent between `MemRefType` and `UnrankedMemrefType`. Fixes #3474

dialects: (linalg) add hidden region to BroadcastOp (#3840)

a216535

Resolves pooling in #2959 , will be tested in #3837

dialects: (linalg) let ConvOpsBase inherit from NamedOpsBase (#3841)

41b16cd

This enables to correct printing of the hidden regions in generic format Resolves conv ops in #2959, will be tested with #3837

dialects: (builtin) remove AnyIntegerAttrConstr (#3842)

617303c

`IntegerAttr` and `IntegerAttr.constr()` are sufficient

dialects: (builtin) remove AnyIntegerAttr (#3843)

51db69f

s/AnyIntegerAttr/IntegerAttr/g

dialects: (builtin) remove AnyFloatAttr(Constr)? (#3844)

4504a60

Adds a default type to `FloatAttr` and removes `AnyFloatAttr` and `AnyFloatAttrConstr`

installation: fix packages discovery (#3847)

6e51e9b

Partial revert of 27bf821

core: Fix multiline error printing (#3849)

34ab330

Fix the MultipleSpanParseError to print multiple lines in the error output --------- Co-authored-by: Alex Rice <[email protected]>

update line numbers in filecheck

4088e1b

Merge branch 'main' into emma/add_fix_forward_ssa_refs

64b9606

alexarice reviewed Feb 6, 2025

View reviewed changes

alexarice approved these changes Feb 6, 2025

View reviewed changes

alexarice reviewed Feb 6, 2025

View reviewed changes

move check to before the code

2291eb9

compor approved these changes Feb 6, 2025

View reviewed changes

compor self-requested a review February 6, 2025 10:47

fix last change to move correct check above code

b356946

emmau678 changed the title ~~fix: change forward block refs to forward ssa refs~~ core: Fix check for forward refs by checking for ssa refs instead of block refs Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Fix check for forward refs by checking for ssa refs instead of block refs #3851

core: Fix check for forward refs by checking for ssa refs instead of block refs #3851

emmau678 commented Feb 6, 2025 •

edited

Loading

review-notebook-app bot commented Feb 6, 2025

emmau678 commented Feb 6, 2025

alexarice Feb 6, 2025

emmau678 Feb 6, 2025

compor Feb 6, 2025 •

edited

Loading

alexarice Feb 6, 2025

superlopuh Feb 6, 2025

compor Feb 6, 2025

superlopuh Feb 7, 2025

alexarice Feb 6, 2025

emmau678 Feb 6, 2025

alexarice Feb 6, 2025

compor Feb 6, 2025

alexarice Feb 6, 2025

superlopuh Feb 6, 2025

codecov bot commented Feb 6, 2025 •

edited

Loading

compor Feb 6, 2025

compor Feb 6, 2025 •

edited

Loading

compor commented Feb 6, 2025

compor commented Feb 6, 2025

core: Fix check for forward refs by checking for ssa refs instead of block refs #3851

Are you sure you want to change the base?

core: Fix check for forward refs by checking for ssa refs instead of block refs #3851

Conversation

emmau678 commented Feb 6, 2025 • edited Loading

review-notebook-app bot commented Feb 6, 2025

emmau678 commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compor Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

compor Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

compor commented Feb 6, 2025

compor commented Feb 6, 2025

emmau678 commented Feb 6, 2025 •

edited

Loading

compor Feb 6, 2025 •

edited

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

compor Feb 6, 2025 •

edited

Loading