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

Update cast_from_pyobject to throw on unsupported types rather than returning null #451

Conversation

dagardner-nv
Copy link
Contributor

@dagardner-nv dagardner-nv commented Mar 11, 2024

Description

  • Currently when cast_from_pyobject encounters an unsupported type it returns a json null.
  • Updates the method to throw a pybind11::type_error, matching the TypeError exception raised by the Python std json.dumps method.
  • Add get_py_type_name helper method
  • Breaking behavior change

Closes #450

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@dagardner-nv dagardner-nv requested a review from a team as a code owner March 11, 2024 19:54
@dagardner-nv dagardner-nv self-assigned this Mar 11, 2024
@dagardner-nv dagardner-nv added bug Something isn't working breaking Breaking change labels Mar 11, 2024
@dagardner-nv dagardner-nv changed the title Update cast_from_pyobject to throw on unsupported types rather than returning None Update cast_from_pyobject to throw on unsupported types rather than returning null Mar 11, 2024
python/mrc/_pymrc/src/utils.cpp Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Merging #451 (610c58c) into branch-24.03 (2dbd985) will decrease coverage by 0.01%.
The diff coverage is 95.23%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-24.03     #451      +/-   ##
================================================
- Coverage         73.81%   73.80%   -0.01%     
================================================
  Files               398      398              
  Lines             14228    14239      +11     
  Branches           1108     1111       +3     
================================================
+ Hits              10502    10509       +7     
- Misses             3726     3730       +4     
Flag Coverage Δ
cpp 70.34% <100.00%> (+<0.01%) ⬆️
py 41.87% <28.57%> (-0.02%) ⬇️

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

Files Coverage Δ
python/mrc/_pymrc/src/utils.cpp 85.54% <95.23%> (+3.72%) ⬆️

... and 5 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@dagardner-nv
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit a920644 into nv-morpheus:branch-24.03 Mar 12, 2024
18 checks passed
@dagardner-nv dagardner-nv deleted the david-cast_from_pyobject-error branch March 12, 2024 21:01
dagardner-nv added a commit to dagardner-nv/MRC that referenced this pull request Apr 4, 2024
rapids-bot bot pushed a commit to nv-morpheus/Morpheus that referenced this pull request Apr 5, 2024
* Fixes an issue where the `--start_time` flag was being ignored
* Prior to nv-morpheus/MRC#451 the `datetime` objects being stored in the config were being serialized to json null, after that change an exception was raised.

Closes #1590

## By Submitting this PR I confirm:
- I am familiar with the [Contributing Guidelines](https://github.com/nv-morpheus/Morpheus/blob/main/docs/source/developer_guide/contributing.md).
- When the PR is ready for review, new or existing tests cover these changes.
- When the PR is ready for review, the documentation is up to date with these changes.

Authors:
  - David Gardner (https://github.com/dagardner-nv)

Approvers:
  - Michael Demoret (https://github.com/mdemoret-nv)

URL: #1592
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: mrc::pymrc::cast_from_pyobject should throw an exception rather than returning a None
2 participants