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

Subtype: avoid some false alarm in subtype_unionall #47868

Closed
wants to merge 3 commits into from

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Dec 11, 2022

The current check is not correct if we set the typevar's bounds to an unwrapped UnionAll. Use var_occurs_inside_skip to skip the inside check.
close #24333, close #47654

@N5N3 N5N3 added the types and dispatch Types, subtyping and method dispatch label Dec 11, 2022
@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 12, 2022

I will try to review and understand this at some point. In the meantime, I thought you might be interested in #47813 and #47877 also, since they also deal with subtyping/intersection.

@vtjnash vtjnash added the needs pkgeval Tests for all registered packages should be run with this change label Dec 12, 2022
@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 15, 2022

@nanosoldier runtests(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

The current check is not correct if we set the typevar's bounds to an unwrapped `UnionAll`. Use `var_occurs_inside_skip` to skip the inside check.
@N5N3
Copy link
Member Author

N5N3 commented Dec 16, 2022

Failure in Unitful shows that b5f7982 fails to pick the right env.
So perhaps we'd better avoid the UnionAll unwrapping when we meet Union with typevar.

@N5N3
Copy link
Member Author

N5N3 commented Dec 16, 2022

@nanosoldier runtests(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@N5N3
Copy link
Member Author

N5N3 commented Dec 17, 2022

The result looks good. The only concern is MosekTools, its test passed on my PC with a local solver, but I saw some random error if I use the network fallback solver.

Since the "Segmentation fault" was happened in libmosek64, I thought this PR might be unrelated? cc @blegat @ulfworsoe

@blegat
Copy link
Contributor

blegat commented Dec 17, 2022

I would assume it is not related to this PR Indeed, @ulfworsoe might know better where this crash is coming from, see https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/2bac38a_vs_b6f32bc/MosekTools.primary.log

@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 20, 2022

As I look further into this, I think the concept of var_occurs_inside and especially in_union is somewhat flawed. I made a counter-example of this here: #31167 (comment)

My current theory is that we need to instead deal with this by setting the var in the env to something unsatisfyable (either with a bit, or with a falsifiable constraint such as Any <: ub <: Union{}) whenever computing the intersection for the lb and ub

@ulfworsoe
Copy link

@blegat It is possible that the crash is solely a MOSEK issue. I don't see how it would be related to a anything on the Julia side, unless it's a memory corruption. We have also seen segfaults when allocation fails, e.g. if a memory limit was set with ulimit.

@JeffBezanson
Copy link
Sponsor Member

I may have a fix for #47658 that also fixes these cases.

@N5N3 N5N3 closed this Jan 24, 2023
@N5N3 N5N3 deleted the subtype_fix1 branch January 24, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs pkgeval Tests for all registered packages should be run with this change types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in subtyping resolution subtyping regression
6 participants