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

Upgrade to dev core versions #316

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

CasperWA
Copy link
Contributor

@CasperWA CasperWA commented Oct 21, 2024

Description:

Along with the summary by Copilot, I can add that I have collected test functions that were split between different test files into a single file that represent tests for the given strategy.

Furthermore, for the tests, I have tried to keep the intent of storing output locally, while making use of pytest fixtures properly and have better test isolation. The latter is achieved by wrapping "raw" script test code in test functions.

Important changes

The settings strategy functions very slightly different due to the new way session objects are handled in a pipeline.
The way it works now is when a pipeline is executed, the configuration key in a configuration is updated with all fields that exist currently in the session object. The session object only exists on the side of the OTEAPI service (or locally as a dict when using the "python" OTEClient).
So, to make this work as intended, the settings strategy still has a settings field, but it will result in a dlite_settings field in the session, and thereby a dlite_settings field in other strategies that rely on these settings.
This new dlite_settings fields has been added to the new DliteConfiguration model, which all strategy-specific configurations that relied on this feature are now based on (except the settings configuration, naturally).
Otherwise, the strategy-specific configurations are based on DliteResult, which is a small model that just includes the collection_id field.

Summary by Copilot

This pull request includes significant changes to the oteapi_dlite package, focusing on refactoring data models and strategies to improve consistency and functionality. The most important changes include the introduction of new data models, the replacement of DLiteSessionUpdate with DLiteResult, and updates to various strategy classes to use these new models.

Data Model Refactoring:

  • Introduced DLiteResult and DLiteConfiguration data models in oteapi_dlite/models.py, which are now used across various strategies for returning values and configurations. (oteapi_dlite/models.py - oteapi_dlite/models.pyR1-R35)

Strategy Class Updates:

  • Replaced DLiteSessionUpdate with DLiteResult in DLiteConvertStrategy, DLiteFilterStrategy, DLiteGenerateStrategy, and DLiteMappingStrategy to standardize result handling. (oteapi_dlite/strategies/convert.py - [1] [2] [3] [4]; oteapi_dlite/strategies/filter.py - [5] [6] [7] [8]; oteapi_dlite/strategies/generate.py - [9] [10] [11] [12] [13] [14] [15] [16]; oteapi_dlite/strategies/mapping.py - [17] [18] [19]

Documentation Updates:

  • Updated documentation to reflect the new models and their usage, including changes in docs/api_reference/models.md, docs/api_reference/models/.pages, and docs/api_reference/models/session.md. (docs/api_reference/models.md - [1]; docs/api_reference/models/.pages - [2]; docs/api_reference/models/session.md - [3]

Code Cleanup:

  • Removed the obsolete DLiteSessionUpdate class and its references from the codebase. (oteapi_dlite/models/session.py - [1]; oteapi_dlite/models/__init__.py - [2]

These changes enhance the consistency and maintainability of the oteapi_dlite package by standardizing data models and simplifying strategy implementations.

Type of change:

  • Bug fix.
  • New feature.
  • Documentation update.

Checklist for the reviewer:

This checklist should be used as a help for the reviewer.

  • Is the change limited to one issue?
  • Does this PR close the issue?
  • Is the code easy to read and understand, including clearly named variables?
  • Do all new feature have an accompanying new test?
  • Has the documentation been updated as necessary?

Rewrite tests to support the new strategy protocol.
The origin has update dev tools and more. These have an influence on the
currently changed files and tests.

Everything has now been updated to comply with the updated dev tools.
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 88.48168% with 22 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@40e1d1f). Learn more about missing BASE report.

Files with missing lines Patch % Lines
oteapi_dlite/strategies/parse_image.py 77.77% 8 Missing ⚠️
oteapi_dlite/strategies/parse_excel.py 87.50% 4 Missing ⚠️
oteapi_dlite/utils/utils.py 71.42% 4 Missing ⚠️
oteapi_dlite/strategies/generate.py 88.23% 2 Missing ⚠️
oteapi_dlite/strategies/mapping.py 87.50% 2 Missing ⚠️
oteapi_dlite/strategies/parse.py 93.54% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #316   +/-   ##
=========================================
  Coverage          ?   88.30%           
=========================================
  Files             ?       15           
  Lines             ?      607           
  Branches          ?        0           
=========================================
  Hits              ?      536           
  Misses            ?       71           
  Partials          ?        0           

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

@CasperWA CasperWA linked an issue Oct 21, 2024 that may be closed by this pull request
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.

Support latest dev version of OTEAPI-Core
1 participant