Skip to content

Add backwards-compat fallbacks for old jac_lin* signatures#515

Closed
arnavk23 wants to merge 3 commits into
JuliaSmoothOptimizers:mainfrom
arnavk23:backcompat/jac_lin-signatures
Closed

Add backwards-compat fallbacks for old jac_lin* signatures#515
arnavk23 wants to merge 3 commits into
JuliaSmoothOptimizers:mainfrom
arnavk23:backcompat/jac_lin-signatures

Conversation

@arnavk23

@arnavk23 arnavk23 commented Nov 18, 2025

Copy link
Copy Markdown
Contributor

In relation with backwards-compat broken by merging of #501.

@arnavk23

Copy link
Copy Markdown
Contributor Author

@amontoison all checks pass.

@amontoison

Copy link
Copy Markdown
Member

@klamike Can you review this PR and check if this PR does what you wanted?

Thanks @arnavk23 for working on this!

Comment thread src/nlp/api.jl Outdated
Comment thread src/nlp/api.jl Outdated
Comment thread src/nlp/api.jl
Comment thread src/nlp/api.jl Outdated
Comment thread src/nlp/api.jl Outdated
Comment thread src/nlp/api.jl
Comment thread src/nlp/api.jl Outdated
@klamike

klamike commented Nov 18, 2025

Copy link
Copy Markdown
Collaborator

ref #514

It seems the QuadraticModels tests are still failing with this branch of NLPModels.jl. You can even see it in the (marked passed!) logs of the Breakage test: https://github.com/JuliaSmoothOptimizers/NLPModels.jl/actions/runs/19462261919/job/55689643511?pr=515#step:6:570

EDIT: seems to be due to lack of permissions on that action, so the PR comment never happens for external PRs. not sure if this should be considered a bug?

@arnavk23

Copy link
Copy Markdown
Contributor Author

ref #514

It seems the QuadraticModels tests are still failing with this branch of NLPModels.jl. You can even see it in the (marked passed!) logs of the Breakage test: https://github.com/JuliaSmoothOptimizers/NLPModels.jl/actions/runs/19462261919/job/55689643511?pr=515#step:6:570

EDIT: seems to be due to lack of permissions on that action, so the PR comment never happens for external PRs. not sure if this should be considered a bug?

As you wrote in EDIT, this is expected GitHub security behavior, not a bug in NLPModels.jl. Workflows triggered by external forks run with a restricted context so they cannot safely use repository secrets or perform write actions on the repository. That’s why the Breakage job can run tests for an external PR but the final “PR comment” step never actually posts a comment.

@tmigot

tmigot commented Nov 18, 2025

Copy link
Copy Markdown
Member

ref #514

It seems the QuadraticModels tests are still failing with this branch of NLPModels.jl. You can even see it in the (marked passed!) logs of the Breakage test: https://github.com/JuliaSmoothOptimizers/NLPModels.jl/actions/runs/19462261919/job/55689643511?pr=515#step:6:570

EDIT: seems to be due to lack of permissions on that action, so the PR comment never happens for external PRs. not sure if this should be considered a bug?

Breakage always show a success in the Actions whenever they pass. Only the comment in the PR will show the real status. Indeed, we are having a bit of an issue with it right now. There is an issue about that already.

@tmigot

tmigot commented Nov 18, 2025

Copy link
Copy Markdown
Member

@arnavk23 can you test the unit tests of QuadraticModels with this PR ? We are probably missing @deprecate macros somewhere or got some wrong.

@tmigot

tmigot commented Nov 20, 2025

Copy link
Copy Markdown
Member

I tried myself and realize we can't really do more. I am closing this and opening a PR to make a new release.

@tmigot tmigot closed this Nov 20, 2025
@arnavk23 arnavk23 deleted the backcompat/jac_lin-signatures branch November 20, 2025 12:05
@arnavk23

Copy link
Copy Markdown
Contributor Author

@tmigot Are you also closing the issue or are we letting it stay there. Also can you review my previous prs on this repo.

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.

4 participants