fix: allow multiple entries for ModelCard considerations lists in xml #744
Conversation
… (so it matches the json spec) Signed-off-by: wievdndr <[email protected]>
Signed-off-by: wievdndr <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the XML schema incorrectly restricted four ModelCard.Considerations list fields to a single entry. The XML schemas (versions 1.5, 1.6, and 1.7) are updated to allow multiple entries for users, useCases, technicalLimitations, and performanceTradeoffs, bringing them into alignment with the JSON schema definition which already defines these as arrays.
Changes:
- Updated XML schema versions 1.5, 1.6, and 1.7 to allow multiple entries (
maxOccurs="unbounded") for four ModelCard.Considerations list fields - Incremented the schema version attributes (patch-level) to reflect the schema change
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| schema/bom-1.7.xsd | Changed maxOccurs from "1" to "unbounded" for user, useCase, technicalLimitation, and performanceTradeoff elements; bumped version from 1.7.0 to 1.7.1 |
| schema/bom-1.6.xsd | Changed maxOccurs from "1" to "unbounded" for user, useCase, technicalLimitation, and performanceTradeoff elements; bumped version from 1.6.1 to 1.6.2 |
| schema/bom-1.5.xsd | Changed maxOccurs from "1" to "unbounded" for user, useCase, technicalLimitation, and performanceTradeoff elements; bumped version from 1.5.0 to 1.5.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jkowalleck I see Copilot suggested testing for multiple entries for the considerations fields. However, the valid-machine-learning test files don't currently test multiple entries for any array fields, all have only single entries (input, output, performanceMetric, ethicalConsideration, fairnessAssessment, etc.). Adding multiple entries just for these four considerations fields would be inconsistent with the overall test structure. I'd suggest opening a separate issue to expand multiple entry testing throughout valid-machine-learning-*.xml, rather than doing this just for these four fields. This keeps the PR focused on the schema fix and addresses array testing systematically. |
we need those regression fixes anyway - and since this appears to be the fix, the tests should be updated here, too - wight? |
Sure, so in the valid-machine-learning test files, should I add multiple entries only for the four considerations fields touched by this fix, or would you prefer that I update the tests to include multiple entries for all fields that allow multiple values, such as inputs, outputs, performanceMetrics, ethicalConsiderations, etc., in this PR? |
performanceTradeoffs in valid-machine-learning-*.xml test files to verify the schema correctly validates multiple entries for these fields. Signed-off-by: wievdndr <[email protected]>
|
(already added second entry for users, useCases, technicalLimitations, and |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
As discussed in ticket #737, the XML schema incorrectly restricts several ModelCard.Considerations list fields to a single entry. The JSON schema defines these fields as arrays, and other ModelCard list fields already allow multiple entries.
This PR updates the XML schemas (1.5, 1.6, 1.7) so the following elements can contain multiple entries:
users/useruseCases/useCasetechnicalLimitations/technicalLimitationperformanceTradeoffs/performanceTradeoffAdditionally, the XML schema version attributes are bumped (patch-only) to reflect the schema change.
fixes #737