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

Open json files as ModelLibrary if romancal is installed #389

Merged
merged 6 commits into from
Sep 26, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Sep 23, 2024

Closes #267

As discussed in the issue, roman_datamodels by itself cannot open an association. So if a user provides an association to roman_datamodels.open we could:

  • raise a helpful exception saying use romancal
  • if a user has romancal installed, open the file as a `ModelLibrary

This PR takes the second approach. Adding support to open .json files provided to datamodels.open as ModelLibrary instances (if romancal is installed).

This required added a new CI job with_romancal which currently only runs the one unit test for this feature.

Regtests show same failures as main: https://github.com/spacetelescope/RegressionTests/actions/runs/11000547719

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • start a romancal regression test with this branch installed ("git+https://github.com/<fork>/roman_datamodels@<branch>")
news fragment change types...
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.misc.rst: infrastructure or miscellaneous change

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.42%. Comparing base (087a60d) to head (93574c6).
Report is 49 commits behind head on main.

Files with missing lines Patch % Lines
src/roman_datamodels/datamodels/_utils.py 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
- Coverage   97.56%   97.42%   -0.14%     
==========================================
  Files          30       37       +7     
  Lines        2788     3380     +592     
==========================================
+ Hits         2720     3293     +573     
- Misses         68       87      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@braingram braingram force-pushed the open_asn branch 8 times, most recently from 4bc5276 to 7ac9fa6 Compare September 23, 2024 17:31
@braingram braingram changed the title Open asn Open json files as ModelLibrary if romancal is installed Sep 23, 2024
@braingram braingram force-pushed the open_asn branch 4 times, most recently from 99aa08a to ad3e5db Compare September 23, 2024 20:00
@braingram braingram marked this pull request as ready for review September 23, 2024 20:30
@braingram braingram requested a review from a team as a code owner September 23, 2024 20:30
Copy link
Collaborator

@PaulHuwe PaulHuwe left a comment

Choose a reason for hiding this comment

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

LGTM with one question

pytest-cov
commands_pre =
bash -c "pip freeze -q | grep 'roman_datamodels @' > {env_tmp_dir}/requirements.txt"
pip install git+https://github.com/spacetelescope/romancal.git
Copy link
Collaborator

Choose a reason for hiding this comment

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

My only question is if this should be against the dev version or the last stable version? dev feels right, but I want to make sure you agree.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree I think dev is most useful here since the versions tend to march along together.

@braingram braingram enabled auto-merge (squash) September 26, 2024 20:02
@braingram braingram merged commit ce9eb29 into spacetelescope:main Sep 26, 2024
15 of 16 checks passed
@braingram braingram deleted the open_asn branch September 26, 2024 20:26
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.

Unable to open association file with datamodels.open
2 participants