Test completion schema wrapper by rjoussen · Pull Request #134 · 4C-multiphysics/fourcipp

rjoussen · 2026-05-29T14:04:38Z

Relies on merging and nightly update of #135.

Changes:

Does all the python changes necessary to also test a schema wrapper referencing another schema. Some of these changes were also already required in Add a completion-oriented schema wrapper 4C#2006. The resolution of required section in the completion schema needed special treatment, so I adapted that.
Add two new configs referencing the completion schema, one for the completion schema that lives in fourcipp, one for the 4C docker container.
Use these configs for testing (PR and nightly)
Add a current completion schema. This is just to see if the normal non-docker tests are passing, which they do. The docker test relies on the updated 4C docker container. After Download completion schema from 4C #135, this last commit is no longer required.

davidrudlstorfer · 2026-05-29T14:32:32Z

Thanks for directly looking into it:)

I do not know if you have already accounted for it but I think this also needs to be updated .github/workflows/update_4C_metadata_schema_file.yaml?

EDIT: correct link https://github.com/4C-multiphysics/fourcipp/blob/main/.github/workflows/update_4C_metadata_schema_file.yaml

rjoussen · 2026-05-29T14:47:06Z

@davidrudlstorfer Yes, I am aware of that. This PR does everything else except downloading the file. This is why this PR includes its own completion schema still. I mainly wanted to check that testing works on all OS etc., before I download the file in the nightly test, revert the last commit of this PR and then re-run the tests here which then should work.

rjoussen · 2026-05-30T12:00:56Z

I ended up moving the schema required section resolution and, more importantly, validator creation to configuration.py, so they only happen once during testing. This speeds up the tests significantly and was necessary for the completion schema because validator creation took a long time, which is a problem since it happened for every 4C input file again in the 4C tests.

Now ready to merge as soon as the completion schema gets updated.

davidrudlstorfer · 2026-05-30T12:21:34Z

Thanks for also looking into the performance, do you have a clue why the code checks, i.e., typo check fails?

rjoussen · 2026-05-30T15:32:13Z

The pre-commit wanted to modify the completion schema included in this PR, which obviously makes no sense. Since this will vanish before merging anyway in favor of the then downloaded completion schema, it's no concern.

If the completion schema download works tonight, I will remove the last commit of this PR, then all tests should pass.

Copilot

Pull request overview

This PR updates FourCIPP’s validation and test configuration to support running the test suite against a “completion-oriented” JSON schema (including the case where that schema is a wrapper referencing another schema), and wires these new profiles into CI so both schemas get exercised.

Changes:

Prebuild and store jsonschema_rs validators on the active ConfigProfile, including a “sections-only” variant with top-level requiredness removed.
Add new config profiles for completion schemas (local and docker) and run pytest under those profiles in GitHub Actions.
Adjust sorting/ordering logic and tests to use resolved required_sections instead of directly reading schema["required"].

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
tests/fourcipp/test_fourc_input.py	Expands docker-only tests to run under an additional docker completion-schema profile; uses resolved required sections in ordering test.
src/fourcipp/utils/validation.py	Changes validation helper to accept a prebuilt `jsonschema_rs.Validator` instead of building from a schema dict each call.
src/fourcipp/utils/configuration.py	Adds resolved required-sections handling, builds/stores validators, and introduces helper methods for wrapper schemas.
src/fourcipp/fourc_input.py	Uses `CONFIG.required_sections` for ordering and switches validation to use prebuilt validators (removes per-call schema munging).
src/fourcipp/config/config.yaml	Adds completion-schema profiles for local and docker use.
.github/workflows/run_testsuite.yaml	Runs pytest twice (full schema + completion schema) and adds docker job coverage for the completion-schema profile.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

rjoussen · 2026-05-31T10:33:51Z

All tests pass using the completion schema from last night. Now ready for review @davidrudlstorfer

davidrudlstorfer

@rjoussen thanks a lot for also updating everything here in FourCIPP. The code changes all look good and ready to merge.

I only have one last comment/favour: I compared the performance results of the last run with your changes to the performance (timings) of the last scheduled run and the new changes worsened nearly all timings across the board.

The performance of reading and dumping files should stay rather consistent. Especially with large meshes it's important that we do not get any performance degradation. (For example in BeamMe we test dumping of large meshes/input files with FourCIPP)

rjoussen · 2026-06-01T14:14:22Z

I only have one last comment/favour: I compared the performance results of the last run with your changes to the performance (timings) of the last scheduled run and the new changes worsened nearly all timings across the board.

Could you point me to which exact timings you are comparing? Are they something directly within the tests? The pipeline in total now takes significantly longer which is expected though, since we're testing everything twice.

davidrudlstorfer · 2026-06-01T14:35:32Z

Could you point me to which exact timings you are comparing? Are they something directly within the tests? The pipeline in total now takes significantly longer which is expected though, since we're testing everything twice.

https://github.com/4C-multiphysics/fourcipp/actions/runs/26708126931

rjoussen · 2026-06-01T16:51:30Z

Okay, I solved it, the timings of this PR are now (ubuntu-latest, python 3.13):

#134 https://github.com/4C-multiphysics/fourcipp/actions/runs/26774729688?pr=134

Operation	Time (seconds)
add_elements	7.138707
validate	3.273528
dump	5.095583
load_from_file	9.695300

versus the last nightly test:

https://github.com/4C-multiphysics/fourcipp/actions/runs/26749752814

Operation	Time (seconds)
add_elements	7.511735
validate	3.573696
dump	5.098570
load_from_file	10.174228

The issue was likely that I unnecessarily stored a full 4C schema in memory. The expensive full validator is still built centrally in configuration.py, but now only lazily on the first call.

This also moves validator creation and schema resolution of requied sections to configuration.py, such that it only happens once during testing.

rjoussen · 2026-06-01T19:06:01Z

Okay, the performance deterioration is removed now. Technically, this is ready to merge now, but it seems fourcipp wants all previous matrix tests to pass. That's quite a bad setup, imo, because it makes it really hard to change anything in the test matrix. The reason I changed it is that the full and completion schema should have separate tests, so we can see performance separately (and not just the last performance as before). I changed it to an ensure_all_tests_pass now, similar to 4C, but that in turn requires changing the required status checks for PRs. Right now, it still expects the old test names, which have been renamed.

davidrudlstorfer · 2026-06-01T19:25:05Z

Okay, the performance deterioration is removed now. Technically, this is ready to merge now, but it seems fourcipp wants all previous matrix tests to pass. That's quite a bad setup, imo, because it makes it really hard to change anything in the test matrix. The reason I changed it is that the full and completion schema should have separate tests, so we can see performance separately (and not just the last performance as before). I changed it to an ensure_all_tests_pass now, similar to 4C, but that in turn requires changing the required status checks for PRs. Right now, it still expects the old test names, which have been renamed.

Yes there's no easy way to prevent this. There is the possibility to set it up like in 4C with a dedicated "all tests passed" stage but this is more work to setup. I can update the required tests tomorrow or simply ignore the check as an admin.

I will have a closer look at the PR tomorrow, and already thanks for looking into the performance as well :)

davidrudlstorfer

Big thanks for the nice PR and also that you further looked into the performance of everything :)

Maybe it would be possible that you quickly request a review from Copilot (somehow I am not able to re-request it on this PR) and then we merge :)

davidrudlstorfer · 2026-06-02T07:11:08Z

PS: I've updated the required status checks for the PR based on your new "ensure_all_tests_pass" :)

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

rjoussen mentioned this pull request May 29, 2026

Download completion schema from 4C #135

Merged

rjoussen self-assigned this May 29, 2026

rjoussen force-pushed the test-completion-schema-wrapper branch 4 times, most recently from de24138 to 5d67d9c Compare May 30, 2026 11:47

rjoussen force-pushed the test-completion-schema-wrapper branch from 5d67d9c to 7f5cab6 Compare May 31, 2026 08:12

rjoussen marked this pull request as ready for review May 31, 2026 08:30

Copilot AI review requested due to automatic review settings May 31, 2026 08:30

Copilot started reviewing on behalf of rjoussen May 31, 2026 08:30 View session

Copilot AI reviewed May 31, 2026

View reviewed changes

rjoussen force-pushed the test-completion-schema-wrapper branch from 7f5cab6 to a5b3142 Compare May 31, 2026 08:49

rjoussen requested a review from davidrudlstorfer May 31, 2026 10:33

davidrudlstorfer reviewed Jun 1, 2026

View reviewed changes

rjoussen force-pushed the test-completion-schema-wrapper branch 2 times, most recently from a714b29 to 73025a6 Compare June 1, 2026 16:29

rjoussen closed this Jun 1, 2026

rjoussen reopened this Jun 1, 2026

rjoussen force-pushed the test-completion-schema-wrapper branch 2 times, most recently from 6ad585e to 7d8f607 Compare June 1, 2026 18:26

feat: Allow testing of completion wrapper schema

2409b1c

This also moves validator creation and schema resolution of requied sections to configuration.py, such that it only happens once during testing.

rjoussen added 4 commits June 1, 2026 20:43

Add configs for completion schema

d2319c5

Run testsuite with 4C completion schema

a7c11b8

feat: Update testsuite to run configs in matrix

004fdf7

Add ensure_all_tests_pass

67787ab

rjoussen force-pushed the test-completion-schema-wrapper branch from 7d8f607 to 67787ab Compare June 1, 2026 18:43

davidrudlstorfer approved these changes Jun 2, 2026

View reviewed changes

Comment thread .github/workflows/run_testsuite.yaml

rjoussen requested a review from Copilot June 2, 2026 07:12

Copilot started reviewing on behalf of rjoussen June 2, 2026 07:12 View session

Copilot AI reviewed Jun 2, 2026

View reviewed changes

Comment thread .github/workflows/run_testsuite.yaml

Comment thread .github/workflows/run_testsuite.yaml

feat: Use bash for all os version when testing

f90ed35

davidrudlstorfer approved these changes Jun 2, 2026

View reviewed changes

rjoussen merged commit 3c9b068 into 4C-multiphysics:main Jun 2, 2026
31 checks passed

Conversation

rjoussen commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidrudlstorfer commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjoussen commented May 29, 2026

Uh oh!

rjoussen commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidrudlstorfer commented May 30, 2026

Uh oh!

rjoussen commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rjoussen commented May 31, 2026

Uh oh!

davidrudlstorfer left a comment

Choose a reason for hiding this comment

Uh oh!

rjoussen commented Jun 1, 2026

Uh oh!

davidrudlstorfer commented Jun 1, 2026

Uh oh!

rjoussen commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

#134 https://github.com/4C-multiphysics/fourcipp/actions/runs/26774729688?pr=134

https://github.com/4C-multiphysics/fourcipp/actions/runs/26749752814

Uh oh!

rjoussen commented Jun 1, 2026

Uh oh!

davidrudlstorfer commented Jun 1, 2026

Uh oh!

davidrudlstorfer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidrudlstorfer commented Jun 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rjoussen commented May 29, 2026 •

edited

Loading

davidrudlstorfer commented May 29, 2026 •

edited

Loading

rjoussen commented May 30, 2026 •

edited

Loading

rjoussen commented May 30, 2026 •

edited

Loading

rjoussen commented Jun 1, 2026 •

edited

Loading