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

sparam inlining: Handle undefined sparams #46703

Merged
merged 1 commit into from
Sep 13, 2022
Merged

sparam inlining: Handle undefined sparams #46703

merged 1 commit into from
Sep 13, 2022

Conversation

Keno
Copy link
Member

@Keno Keno commented Sep 11, 2022

_compute_sparams is not required to be able to resolve an sparam, it can leave it as a TypeVar, in which case accessing it should throw an UndefVarError. This needs to be appropriately handled in inlining. We have a test case like this in the test suite, but it was being papered over by an incorrect check in can_inline_typevar. Remove that check and implement proper undef checking in inlining.

@Keno Keno requested a review from ianatol September 11, 2022 02:39
`_compute_sparams` is not required to be able to resolve an sparam,
it can leave it as a `TypeVar`, in which case accessing it should
throw an UndefVarError. This needs to be appropriately handled in
inlining. We have a test case like this in the test suite, but it
was being papered over by an incorrect check in can_inline_typevar.
Remove that check and implement proper undef checking in inlining.
@Keno Keno added the needs pkgeval Tests for all registered packages should be run with this change label Sep 11, 2022
@Keno
Copy link
Member Author

Keno commented Sep 11, 2022

This is subtle enough that we should run a PkgEval over it. Unfortunately, PkgEval is currently broken.

@ianatol
Copy link
Member

ianatol commented Sep 12, 2022

Thanks, I knew that can_inline_typevar check was coarse and incorrect, but wasn't sure how to go about handling the TypeVar case. This looks good to me

@Keno
Copy link
Member Author

Keno commented Sep 13, 2022

@nanosoldier runtests(ALL, configuration = (rr=false,))

@nanosoldier
Copy link
Collaborator

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

@maleadt
Copy link
Member

maleadt commented Sep 13, 2022

I'm looking into the PkgEval issues and will re-trigger when fixed.

@maleadt
Copy link
Member

maleadt commented Sep 13, 2022

Turns out the .julia/packages store had been corrupted. In addition, I disabled rr to work around the FilePathsBase issue. Let's re-trigger (also assuming you wanted a comparison build):

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

@Keno
Copy link
Member Author

Keno commented Sep 13, 2022

@nanosoldier runtests(["ADI", "AlphaStableDistributions", "AuditoryStimuli", "BloqadeKrylov", "Causal", "ConsistencyResampling", "DeepQLearning", "DelayDiffEq", "DevIL", "DiagnosisClassification", "Diagonalizations", "DifferentiableStateSpaceModels", "DrelTools", "EHRAuthentication", "ElasticFDA", "FastMarching", "Infernal", "JacobiDavidson", "LCPsolve", "Lasso", "LogicToolkit", "Metalhead", "MetapopulationDynamics", "NeXLCore", "NeXLSpectrum", "NeidArchive", "PalmerPenguins", "PencilArrays", "PolygonOps", "PowerSimulations", "PreprocessMD", "QuantumTomography", "REoptLite", "RPRMakie", "SpectralDistances", "StochasticRounding", "StructJuMP", "Surprise", "SwagUI", "ToQUBO", "Tullio", "Whitebox"], vs = ":master")

@nanosoldier
Copy link
Collaborator

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

@ianatol
Copy link
Member

ianatol commented Sep 13, 2022

PkgEval seems clean

@Keno Keno merged commit f9b362e into master Sep 13, 2022
@Keno Keno deleted the kf/sparamundef branch September 13, 2022 22:48
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants