Skip to content

[ARITH] Fix InternalError: Check failed: (eval_vec_) is false#18536

Merged
tlopex merged 1 commit intoapache:mainfrom
cchung100m:issue-17936
Dec 3, 2025
Merged

[ARITH] Fix InternalError: Check failed: (eval_vec_) is false#18536
tlopex merged 1 commit intoapache:mainfrom
cchung100m:issue-17936

Conversation

@cchung100m
Copy link
Copy Markdown
Contributor

@cchung100m cchung100m commented Dec 1, 2025

Hi Commiters,

This PR is trying to fix issues #17936. Any suggestions would be appreciated if you are available.

Root Cause

Code paths that expected vector evaluation but encounter the scalar-only evaluation eval_vec_ = false

Solution

BroadcastNode just replicates the same scalar value and the evaluation might not requires special vector-aware handling

@cchung100m cchung100m marked this pull request as ready for review December 2, 2025 03:16
Copy link
Copy Markdown
Member

@tlopex tlopex left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

@tlopex tlopex merged commit cd820b5 into apache:main Dec 3, 2025
11 of 12 checks passed
@tqchen
Copy link
Copy Markdown
Member

tqchen commented Dec 3, 2025

would be good to dig into what is going on in the original case, since the old API states that eval_vec_ is an intentionally flag for evaluating vectors, and the CHECK explicitly checks the invariance, turning this off for the one off case may leads to unintentional behavior. cc @tlopex , ideally we should keep the invariance when possible

@cchung100m cchung100m deleted the issue-17936 branch December 3, 2025 15:58
@cchung100m
Copy link
Copy Markdown
Contributor Author

Hi @tqchen

Thanks for review comments and point out the crucial part and potential risk. I wasn't thinking straight to resolve the issue. Should we revert this change?

@tqchen
Copy link
Copy Markdown
Member

tqchen commented Dec 3, 2025

i think reverting for now is safer

tlopex added a commit that referenced this pull request Dec 3, 2025
@tlopex
Copy link
Copy Markdown
Member

tlopex commented Dec 3, 2025

@tqchen Thanks for pointing that. I'll send a pr to revert it now.

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