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

Ensure no validation when active_validation is False #270

Closed
wants to merge 1 commit into from

Conversation

markheik
Copy link
Collaborator

@markheik markheik commented Oct 19, 2023

Description:

This PR addresses performance issues in #269

JSON schema validation decreases the performance of the function and should be bypassed if
the user does not want the validation and requires performance.

Changed to fastjsonschema which is much faster.


I ran the example in the issue #269 locally and results are for active_validation=True:

Before:

  • with JSON Schema validation: ~10 ms
  • without validation: 0.01 - 0.04 ms

After:

  • with JSON Schema validation: ~0.01 - 0.04 ms
  • without validation: 0.01 - 0.04 ms

Fixes issue: #269

Checklist:

  • Add tests for the change to show correct behavior.
  • Add or update relevant docs, code and examples.
  • Update CHANGELOG.rst with relevant information and add the issue number.
  • Add .. versionchanged:: where necessary.

@markheik
Copy link
Collaborator Author

markheik commented Oct 19, 2023

I am still unsure if we want to acknowledge active_validation value here, but the default should be True.

jsonschema.validate() seems to be slow overall, I will look for a replacement implementation.

There is fastjsonschema which seems to faster.

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (7e7bccb) 99.60% compared to head (0b81ca3) 99.41%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #270      +/-   ##
==========================================
- Coverage   99.60%   99.41%   -0.19%     
==========================================
  Files          40       40              
  Lines        2552     2580      +28     
==========================================
+ Hits         2542     2565      +23     
- Misses         10       15       +5     
Files Coverage Δ
.../zhinst/toolkit/driver/nodes/command_table_node.py 100.00% <100.00%> (ø)
src/zhinst/toolkit/command_table.py 97.58% <87.17%> (-2.42%) ⬇️

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

@markheik markheik self-assigned this Oct 19, 2023
@markheik markheik added the enhancement New feature or request label Oct 19, 2023
@markheik markheik force-pushed the markush/add_validate_flag_to_command_table branch from 6530cb9 to 0b81ca3 Compare October 20, 2023 12:24
@markheik markheik changed the title Add validate argument to CommandTable.as_dict() Ensure no validation when active_validation is False Oct 20, 2023
) -> dict:
"""Carry over parent JSON schema version to the child."""
# Required for `fastjsonschema` validation
child["$schema"] = parent.get("$schema", default)

Check failure

Code scanning / CodeQL

Modification of parameter with default Error

This expression mutates a
default value
.
@markheik
Copy link
Collaborator Author

I will close this in favor of #271 As this is out of scope for #269 .
Will revisit the overall performance improvements later.

@markheik markheik closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant