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

JP-2604: Update ref file schemas with current CRDS selectors #6866

Merged
merged 17 commits into from
Jun 10, 2022

Conversation

tapastro
Copy link
Contributor

@tapastro tapastro commented May 27, 2022

Closes #
Resolves JP-2604

Description

This PR addresses outdated reference file schemas that were lacking structure for current CRDS selectors, causing possible issues when creating new reference file datamodels from scratch. A test has been created but could use some polishing before being added.

Checklist

  • Tests
  • Documentation
  • Change log
  • Milestone
  • Label(s)

Copy link
Collaborator

@stscieisenhamer stscieisenhamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment about setting of the crds environment.

jwst/datamodels/tests/test_schema_against_crds.py Outdated Show resolved Hide resolved
@tapastro
Copy link
Contributor Author

The test now runs locally, with the fix mentioned in the stdatamodels issue filed here. I don't know if the change is at all reasonable, so hoping someone who knows more about that repo can weigh in.

@tapastro tapastro force-pushed the jp-2604-ref-schema-selectors branch from fdd8a32 to ae4bcfd Compare May 31, 2022 15:56
@tapastro
Copy link
Contributor Author

tapastro commented Jun 1, 2022

Pending an stdatamodels release, I'm still working on how to retrieve the files from CRDS during this test. Work on my local machine had initially used either central store or local cache, but the latest commit should cache files in the temporary test environment.

Documentation still a WIP.

@stscieisenhamer stscieisenhamer self-requested a review June 2, 2022 14:49
Copy link
Collaborator

@stscieisenhamer stscieisenhamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes for the CRDS settings looks good here.

Is there still work to be done on this PR?

@tapastro
Copy link
Contributor Author

tapastro commented Jun 2, 2022

The new test is failing in (I believe) the expected fashion, as it's missing the recent PR to stdatamodels. Still working on documentation updates...

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #6866 (f065c42) into master (de64ddc) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6866      +/-   ##
==========================================
+ Coverage   79.32%   79.36%   +0.04%     
==========================================
  Files         418      418              
  Lines       37171    37171              
==========================================
+ Hits        29485    29501      +16     
+ Misses       7686     7670      -16     
Flag Coverage Δ
nightly 79.35% <ø> (+0.04%) ⬆️
unit 53.42% <ø> (ø)
Impacted Files Coverage Δ
jwst/regtest/conftest.py 73.36% <0.00%> (+8.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de64ddc...f065c42. Read the comment docs.

@tapastro tapastro force-pushed the jp-2604-ref-schema-selectors branch from 92017cc to 858c9fc Compare June 2, 2022 18:52
@tapastro
Copy link
Contributor Author

tapastro commented Jun 2, 2022

With up-to-date stdatamodels, Jenkins run passes successfully: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/334/

Docs updated and build succesfully.

If no release is made for stdatamodels, this will introduce a unit test that fails until a release is made. Besides that, this PR should be good to merge.

Copy link
Collaborator

@stscieisenhamer stscieisenhamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tapastro
Copy link
Contributor Author

Wanted to run one more set of regtests before merging - getting a bunch of errors related to what looks like a set_telescope_pointing update? Is this schema-related? https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/341/#showFailuresLink

@hbushouse
Copy link
Collaborator

Wanted to run one more set of regtests before merging - getting a bunch of errors related to what looks like a set_telescope_pointing update? Is this schema-related? https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/341/#showFailuresLink

SIAF update? Or do we use a locked down version in our tests?

@tapastro
Copy link
Contributor Author

Wanted to run one more set of regtests before merging - getting a bunch of errors related to what looks like a set_telescope_pointing update? Is this schema-related? https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/341/#showFailuresLink

SIAF update? Or do we use a locked down version in our tests?

Ah, of course. New version released three hours ago. That'd do it! Guess someone will have to hit the big red button on those later.

@tapastro tapastro merged commit 64ab7f3 into spacetelescope:master Jun 10, 2022
Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that was a lot of missing keywords. I'm guessing that indicates relatively few people are using our reference file datamodels to actually create reference files.

@@ -1,6 +1,7 @@
1.5.3 (unreleased)
==================


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. Are 2 blank lines required below a release heading?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that might have been the result of a poorly-attempted merge conflict... 😬

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No biggy. Can be fixed in the next PR.

@hbushouse hbushouse modified the milestones: Build 8.1, 8.0.1 Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants