feat: HiFa v1.1.0 schema development #1978

kratsg · 2022-09-01T22:09:27Z

Pull Request Description

The following is happening:

resolves Update validator for jsonschema #1980
start a HiFa JSON v1.1.0, but not release it.
allow for defining a default schema version for the different schemas for pyhf to support in the current implementation
provide a utility to migrate (upgrade/downgrade) provided specifications (workspace, patchset) to a different version
provide command-line interface for upgrading/downgrading
flesh out the type-hints for pyhf.schema

To-Do:

agree on how upgrade/downgrade works internally: separate classes, or single class like suggested by @matthewfeickert in comments, or more like functional form similar to what nbconvert does (see my comment below feat: HiFa v1.1.0 schema development #1978 (comment))
determine whether we allow workspace/model and patchset to have different versions or not, or require them to always be synced (this changes what we do for pyhf.schema.versions to pyhf.schema.version depending...)

Schema updates:

Update from Draft 6 to 2020-12 Draft instead
Support serialization of custom function (another PR)
Support serialization of multiple POIs (another PR)
Support serialization of bin-wise fixed/constant parameters (another PR)
Support HS3's HistFactory schema (another PR)

Checklist Before Requesting Reviewer

Tests are passing
"WIP" removed from the title of the pull request
Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

Summarize commit messages into a comprehensive review of the PR

matthewfeickert · 2022-12-06T22:52:26Z

@kratsg you have this slated for v0.7.1 at the moment. Does this mean that you feel we have a problem that needs to be patched? If not, we should probably place this in v0.8.0.

kratsg · 2022-12-06T22:57:18Z

@kratsg you have this slated for v0.7.1 at the moment. Does this mean that you feel we have a problem that needs to be patched? If not, we should probably place this in v0.8.0.

It's going into the next release though. I don't know if we want to start juggling development HiFa v1.0.0 and v1.1.0 at the same time? But this will need to go in, so that we can deal with multiple schema versions.

matthewfeickert · 2022-12-06T23:10:56Z

It's going into the next release though. I don't know if we want to start juggling development HiFa v1.0.0 and v1.1.0 at the same time? But this will need to go in, so that we can deal with multiple schema versions.

Okay, then I've switched it to be v0.8.0. After the issues with getting v0.7.0 out and also accepting PRs I think it makes sense (for me to figure out how to do release branches) to start using patch releases for when we actually have a bug we need to go back and patch and start having other releases be minor (until we hit v1.0.0).

codecov · 2022-12-07T17:15:36Z

Codecov Report

Base: 98.30% // Head: 98.30% // No change to project coverage 👍

Coverage data is based on head (2b168b7) compared to base (2b168b7).
Patch has no changes to coverable lines.

❗ Current head 2b168b7 differs from pull request most recent head 178b3db. Consider uploading reports for the commit 178b3db to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1978   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          69       69           
  Lines        4531     4531           
  Branches      645      645           
=======================================
  Hits         4454     4454           
  Misses         45       45           
  Partials       32       32

Flag	Coverage Δ
contrib	`97.88% <0.00%> (ø)`
doctest	`61.15% <0.00%> (ø)`
unittests-3.10	`96.31% <0.00%> (ø)`
unittests-3.8	`96.33% <0.00%> (ø)`
unittests-3.9	`96.35% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/pyhf/schema/loader.py

matthewfeickert

Thanks @kratsg for this nice work. Overall this looks good. 👍

I've added a question and some small formatting suggestions, but as we need to fix up the coverage thing I could add those in this weekend once I am at CERN so that you don't waste time having to poke at coverage's decisions. It is probably faster to locally run

$ nox --session tests --python 3.10 -- coverage
$ nox --session coverage
$ xdg-open ./htmlcov/index.html

to debug coverage than to have GHA do all of that and then have Codecov report it back to us.

If you can also fill out the PR body with some additional context that would be helpful, given that you've done a lot of nice work here (I can also summarize this later, but am not able to succinctly do so now given a lot was added to support this).

matthewfeickert · 2022-12-09T22:02:16Z

src/pyhf/schema/upgrader.py

+class Upgrade_1_0_1:
+    """
+    Used for testing functionality of upgrade.
+    """
+
+    version: SchemaVersion = '1.0.1'


Suggested change

class Upgrade_1_0_1:

"""

Used for testing functionality of upgrade.

"""

version: SchemaVersion = '1.0.1'

class Upgrade(version: SchemaVersion = "1.0.1"):

"""

Used for testing functionality of upgrade.

"""

Maybe I'm missing something about what the purpose of upgrader is, but is there a reason why we need to make this class schema v1.0.1 hardcoded?

We need an Upgrade_X_Y_Z class for every single version we plan to support upgrading to. When we add a new version of the spec, we have to have a new upgrade class that knows how to upgrade from an old version to the version the class represents.

@kratsg Ah okay, so the real reason is that we can't deterministically know what version we're upgrading from in advance?

Do we have a plan for how to handle upgrading multiple versions in one go? e.g. Let's say we have v1.0.0, v1.1.0, v1.2.0, and v2.0.0. If at this point someone comes along with a v1.0.0schema and wants to move fromv1.0.0tov2.0.0` what's the plan for doing this?

I have ideas on how to do this with python's mro

matthewfeickert · 2022-12-09T22:03:14Z

src/pyhf/schema/upgrader.py

+def upgrade(*, to_version: SchemaVersion | None = None) -> type[UpgradeProtocol]:
+    if to_version is None or to_version == '1.0.1':
+        return Upgrade_1_0_1


Suggested change

def upgrade(*, to_version: SchemaVersion | None = None) -> type[UpgradeProtocol]:

if to_version is None or to_version == '1.0.1':

return Upgrade_1_0_1

def upgrade(*, to_version: SchemaVersion | None = None) -> type[UpgradeProtocol]:

if to_version is None or to_version == "1.0.1":

return Upgrade("1.0.1")

Follow up to the above.

tests/test_upgrade.py

matthewfeickert · 2022-12-09T22:22:21Z

(Ignore the docs failing as this is Springer's fault: https://twitter.com/HEPfeickert/status/1600915112403304449?s=20&t=kjTW5TNbU_Jukxb0ALd0uQ. We can either fix this up locally in another PR or just wait for them to fix it.)

matthewfeickert

@kratsg sorry just a few more questions to help me understand this better. As this will go in this weekend can I also ask you sometime before Monday to write up a summary in the PR body and the squash and merge summary commit? This is a huge amount of work (:bow:) and I want to make sure that we have a summary of this as I think if I had to try to sit down and reread all of this again late at night I might be pressed to do so without some accompanying text.

matthewfeickert · 2023-02-04T07:06:26Z

src/pyhf/schema/upgrader.py

+class Upgrade_1_0_1:
+    """
+    Used for testing functionality of upgrade.
+    """
+
+    version: SchemaVersion = '1.0.1'


@kratsg Ah okay, so the real reason is that we can't deterministically know what version we're upgrading from in advance?

Do we have a plan for how to handle upgrading multiple versions in one go? e.g. Let's say we have v1.0.0, v1.1.0, v1.2.0, and v2.0.0. If at this point someone comes along with a v1.0.0schema and wants to move fromv1.0.0tov2.0.0` what's the plan for doing this?

matthewfeickert · 2023-02-04T07:06:31Z

tests/test_workspace.py

@@ -453,6 +453,7 @@ def test_combine_workspace_incompatible_poi(workspace_factory, join):
 def test_combine_workspace_diff_version(workspace_factory, join):
    ws = workspace_factory()
    ws.version = '1.0.0'
+    ws['version'] = '1.0.0'


I'm probably missing the point of this test, but if you put a breakpoint just after ws = workspace_factory()

(Pdb) ws.version '1.0.0' (Pdb) ws["version"] '1.0.0'

so is the reason to explicitly set this a means to solidify the versions that will be used in the future?

This is just a test to enforce absolutely different versions.

kratsg · 2023-03-23T16:01:43Z

Look into how nbformat handles converting between versions: (thanks @agoose77 ) which is done by defining a v#/convert.py such as for v2 here.

…ys have it

This reverts commit 0f10984.

… HiFa

kratsg added feat/enhancement New feature or request schema and spec JSON schema type checking Related to types and type checking labels Sep 1, 2022

kratsg self-assigned this Sep 1, 2022

kratsg marked this pull request as draft September 1, 2022 22:10

This was referenced Sep 2, 2022

Update validator for jsonschema #1980

Open

jsonschema RefResolutionError for 0.6.3 or lower #1975

Closed

matthewfeickert changed the base branch from master to main September 21, 2022 20:52

kratsg marked this pull request as ready for review December 6, 2022 21:56

kratsg changed the title ~~feat: Update JSON Schema draft~~ feat: HiFa v1.1.0 schema development Dec 6, 2022

kratsg commented Dec 7, 2022

View reviewed changes

src/pyhf/schema/loader.py Show resolved Hide resolved

kratsg requested a review from matthewfeickert December 9, 2022 05:00

matthewfeickert reviewed Dec 9, 2022

View reviewed changes

kratsg force-pushed the feat/updateJSONSchemaDraft branch from d22e37d to 4e144d7 Compare February 2, 2023 18:20

matthewfeickert reviewed Feb 4, 2023

View reviewed changes

matthewfeickert force-pushed the feat/updateJSONSchemaDraft branch from 4b061d7 to 178b3db Compare February 7, 2023 06:30

matthewfeickert force-pushed the feat/updateJSONSchemaDraft branch from 178b3db to 3a3f3a8 Compare April 7, 2023 06:10

kratsg added 5 commits December 7, 2023 14:12

update validator to latest draft

4934ed9

add v1.0.1

2569988

move load_schema for defs.json as referrer document to ensure we alwa…

bb7b416

…ys have it

add version to typing

3dd5766

add warning when version is wrong

81c5816

kratsg added 24 commits December 7, 2023 14:18

add upgrade CLI

a2f4899

expose api

41a0b23

add tests for the upgrade functionality in python

58c79f0

don't lock patchset and workspace together

7b60642

fix some tests

58fe1b7

fix broken tests

9b8ea83

fix up more tests

691a3b5

fix pre-commit

26a5f32

add typing_extensions

b75ab60

add 1.1.0 and keep latest schema version at 1.0.0

b15850c

fix vers

b91ee76

fix up test

c89eff0

fix typing

8f10cd8

switch to class-based so we can always upgrade between diff versions

b6edb60

fix up tests for now

f854c23

fix up coverage

4f84a65

update version to 1.1.0

06f13a9

update draft

4563ae3

Revert "update draft"

3bb4d0e

This reverts commit 0f10984.

downgrade validator to maintain behavior for correct draft for v1.0.0…

fcfab82

… HiFa

switch to draft/2020-12 default

862aabb

test upgrade 1.0.1 to itself

6249ffa

fix and use context manager

f05cb95

fix coverage

531e0ba

kratsg force-pushed the feat/updateJSONSchemaDraft branch from 3a3f3a8 to 531e0ba Compare December 7, 2023 13:23

kratsg added 4 commits December 7, 2023 15:05

initial fix up of typehints

bc7fffa

fix up

e66d252

Merge branch 'main' into feat/updateJSONSchemaDraft

f1df51f

don't change draft version for 1.0.0

372c8d2

kratsg mentioned this pull request Dec 8, 2023

feat: New schema for HS3 Support #2397

Draft

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: HiFa v1.1.0 schema development #1978

feat: HiFa v1.1.0 schema development #1978

kratsg commented Sep 1, 2022 •

edited

Loading

matthewfeickert commented Dec 6, 2022

kratsg commented Dec 6, 2022

matthewfeickert commented Dec 6, 2022

codecov bot commented Dec 7, 2022 •

edited

Loading

matthewfeickert left a comment

matthewfeickert Dec 9, 2022

kratsg Feb 2, 2023

matthewfeickert Feb 4, 2023

kratsg Dec 7, 2023

matthewfeickert Dec 9, 2022

matthewfeickert commented Dec 9, 2022 •

edited

Loading

matthewfeickert left a comment

matthewfeickert Feb 4, 2023

matthewfeickert Feb 4, 2023

kratsg Dec 7, 2023

kratsg commented Mar 23, 2023 •

edited

Loading

feat: HiFa v1.1.0 schema development #1978

Are you sure you want to change the base?

feat: HiFa v1.1.0 schema development #1978

Conversation

kratsg commented Sep 1, 2022 • edited Loading

Pull Request Description

Checklist Before Requesting Reviewer

Before Merging

matthewfeickert commented Dec 6, 2022

kratsg commented Dec 6, 2022

matthewfeickert commented Dec 6, 2022

codecov bot commented Dec 7, 2022 • edited Loading

Codecov Report

matthewfeickert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewfeickert commented Dec 9, 2022 • edited Loading

matthewfeickert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kratsg commented Mar 23, 2023 • edited Loading

kratsg commented Sep 1, 2022 •

edited

Loading

codecov bot commented Dec 7, 2022 •

edited

Loading

matthewfeickert commented Dec 9, 2022 •

edited

Loading

kratsg commented Mar 23, 2023 •

edited

Loading