Improved performance of write big TimeSerie - #64
jacopocinaark wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Enum serialization can bypass configured names and break enum-key round trips.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves serialization performance for large time-series dictionary payloads.
Changes:
- Adds fast paths for primitive values and datetimes.
- Caches serializers and append operations.
- Delegates complex values to
jsons.dump.
File summaries
| File | Summary |
|---|---|
src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py |
Optimizes dictionary serialization, but enum values and keys require compatibility fixes. |
Review details
Suppressed comments (1)
src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py:70
- This
isinstancefast path also matchesIntEnum/StrEnumvalues, so they bypassjsons.dumpand its configureduse_enum_name=Truebehavior. The resulting payload contains the underlying enum value instead of the enum name, unlike enum fields serialized elsewhere; use exact built-in-type checks or explicitly exclude enum instances.
elif isinstance(v, (str, int, float, bool)) or v is None:
v_out = v
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ARKlab/Artesian.SDK-Python into feature/22400-PythonPerformance It looks like you may be committing a merge.
There was a problem hiding this comment.
the original CODEOWNER is correct. LICENSE, CODEOWNERS, renovate.json and .github/ configuration (workflows, checks, and other .github setup) MUST be vetted by ARKlab/maintainers
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved critical API removals, release and CI workflow defects, and serializer compatibility concerns require further review.
Review details
Suppressed comments (4)
.github/workflows/python-tests.yml:60
- The previous release jobs validated tag shape and that the tagged commit belonged to the required source branch; both validation scripts and checks were removed. This now allows a
vtag on an arbitrary branch or commit to be published as a release. Restore release-tag and source-branch validation before publishing.
if: github.event_name == 'push' && (github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/tags/v'))
.github/workflows/python-tests.yml:45
- Pyright was previously a single job, but placing it inside the 4-Python × 3-OS build matrix runs the same analysis 12 times. This needlessly increases CI time and runner cost; keep static analysis outside the matrix or run it in one dedicated job.
- name: Run Pyright
uses: jordemort/action-pyright@v1
with:
github_token: ${{ secrets.GITHUB_TOKEN }}
reporter: github-check
lib:
level: warning
filter_mode: file
.github/workflows/python-tests.yml:59
- This rewrite removes the coverage artifact upload and consolidated test-report jobs even though pytest is configured to generate junit XML and XML/HTML coverage. CI will no longer publish coverage or a single failure report; preserve equivalent reporting or explicitly accept this observability regression.
publish:
src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py:64
- The old serializer delegated every non-JSON key to
jsons.dump, which honorsuse_enum_nameand registered serializers. Converting arbitrary keys withstrchanges the wire value—for example an enum key becomesMarket.MGPinstead ofMGP—and can make deserialization fail. Keep the primitive fast path, but retainjsons.dumpfor the fallback.
k_out = str(k)
- Files reviewed: 21/22 changed files
- Comments generated: 11
- Review effort level: Lite
c71dff1 to
c529b1b
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Restore configured jsons fallback behavior for non-primitive keys.
Review details
Suppressed comments (1)
src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py:64
- This fallback no longer uses the configured
jsonsserializer for non-primitive keys. For example, withuse_enum_name=True, an enum key that previously serialized to its member name is now converted bystr()toEnumClass.Member, and custom key serializers are bypassed; retain thejsons.dumpfallback for these keys while keeping the datetime fast path.
k_out = str(k)
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The current str(k) fallback changes the existing serialization contract for non-primitive keys.
Review details
Suppressed comments (1)
src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py:64
str(k)changes the existing serialization contract for non-primitive keys. For example, with the configureduse_enum_name=True, the previousjsons.dump(k, key_transformer=None, **kwargs)serialized an enum key asActualTimeSerie, while this emitsMarketDataType.ActualTimeSerie; custom key serializers are also bypassed. Keep thejsons.dumpfallback for this branch so the optimization does not change key values.
k_out = str(k)
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ARKlab/Artesian.SDK-Python into feature/22400-PythonPerformance
There was a problem hiding this comment.
🔵 Needs a closer look
It combines unresolved breaking API removals with release workflow and serializer contract changes.
Review details
Suppressed comments (10)
.github/workflows/python-tests.yml:10
- GitHub Actions
tagsfilters use glob patterns, not regex. The+characters and escaped dots make this pattern fail to match a tag such asv1.2.3, so GA tag pushes will not trigger the workflow. Use a glob-compatible trigger together with explicit tag validation in the publish job.
- 'v[0-9]+\.[0-9]+\.[0-9]+'
.github/workflows/python-tests.yml:60
- This condition publishes to PyPI on every push to
master, including ordinary pull-request merges. That creates unintended development releases (or repeated-version upload failures) and is unrelated to the serializer optimization; publishing should be gated by a validated release tag while normal master pushes remain test/build-only.
if: github.event_name == 'push' && (github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/tags/v'))
.github/workflows/python-tests.yml:38
- Pyright was moved into the matrix job, so it now runs 12 times (four Python versions across three operating systems) instead of once in its dedicated job. This adds redundant work and lengthens every CI run; keep the type-check job outside the OS/Python test matrix.
- name: Run Pyright
.github/workflows/python-tests.yml:17
- The Pyright step still uses the GitHub Checks reporter, but this job no longer grants
checks: write(the previous workflow did). With read-only repository defaults, including forked pull requests, the action cannot create or update its check. Declare the required job permission explicitly.
build:
runs-on: ${{ matrix.os }}
CODEOWNERS:1
- This replaces maintainer-only ownership for
.github/,LICENSE,CODEOWNERS, andrenovate.jsonwith the broad*rule, allowingteam-teapot418to approve changes to repository controls and release automation. Preserve the specific maintainer-only rules for these sensitive paths.
* @ARKlab/team-teapot418 @ARKlab/maintainers
README.md:779
- This code block contains Python syntax but is now labeled as C#, so README renderers apply the wrong syntax highlighting. Keep the language marker consistent with the code.
```csharp
README.md:884
- The table header introduces the typo
REPALACE; useREPLACEto match the upsert mode name used throughout the surrounding documentation.
| DATETIME | EXISTING | PAYLOAD | MERGE | REPALACE |
src/Artesian/GMEPublicOffers/_Enum/Market.py:27
- Removing these public
Marketmembers makes existing queries forGMP_ASTA,GMP_Cont,MI_Cont, andMGSfail before request construction, even though they are valid query filter values represented by this SDK. This unrelated breaking API removal should not be part of a serializer performance change.
AFRE = 200
src/Artesian/GMEPublicOffers/_Enum/Status.py:13
- Removing
Status.DISbreaks existing clients that use this public status filter: attribute lookup fails and the corresponding query mapping is gone. This unrelated breaking API removal should be reverted or handled as an explicitly versioned change.
PREJ = 9
src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py:64
- The fallback now stringifies every non-primitive key. That changes the existing serializer contract: with
use_enum_name=True, an enum key was passed throughjsons.dump(for example,Status.ACCbecomesACC), but this now becomesStatus.ACC; registered serializers for other key types are also bypassed. Keep thejsons.dumpfallback for non-primitive keys.
k_out = str(k)
- Files reviewed: 21/22 changed files
- Comments generated: 5
- Review effort level: Lite
|
|
||
| Attributes: | ||
| derivedAlgorithm: the derived configuration algorithm (MUV, Coalesce, Sum, Transform) | ||
| derivedAlgorithm: the derived configuration algorithm (MUV, Coalesce, Sum) |
c5fa5fd to
c529b1b
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Serializer fast paths may alter enum and custom-key serialization behavior.
Review details
Suppressed comments (2)
src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py:64
- Converting every non-JSON key with
strbypasses the serializer settings. Withuse_enum_name=Trueconfigured below, an Enum key previously serialized throughjsons.dumpas its member name, but now becomes a string such asMarketDataType.ActualTimeSerie; other custom key types can change similarly. Preserve the fallback serializer for non-primitive keys so this optimization does not change the wire format.
k_out = str(k)
src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py:69
isinstancealso matches scalar subclasses such asIntEnumandstr-backed enums, so these values now bypassjsons.dumpand itsuse_enum_name=True/custom serializer handling. The previous implementation dumped every value. Restrict this fast path to exact built-in scalar types so subclass behavior remains unchanged.
elif isinstance(v, (str, int, float, bool)) or v is None:
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ARKlab/Artesian.SDK-Python into feature/22400-PythonPerformance
| /LICENSE @ARKlab/maintainers | ||
| /CODEOWNERS @ARKlab/maintainers | ||
| /renovate.json @ARKlab/maintainers | ||
| * @ARKlab/team-teapot418 @ARKlab/maintainers |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved CI publishing and public API compatibility issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (11)
.github/workflows/python-tests.yml:10
on.push.tagspatterns use GitHub glob syntax, not regular expressions.[0-9]+therefore requires a literal+, so a normal tag such asv1.2.3does not match and the release workflow is skipped. Replace this with a valid glob and restore strict tag validation.
- 'v[0-9]+\.[0-9]+\.[0-9]+'
.github/workflows/python-tests.yml:57
- The new workflow stops uploading
coverage.xmland removes the aggregate coverage/test-report jobs, even though pytest still generates coverage by default. This removes coverage visibility from CI; retain a coverage artifact/report if it is part of the repository's validation.
- name: Upload pytest test results
uses: actions/upload-artifact@v7
with:
name: pytest-results-${{ matrix.python-version }}-${{ matrix.os }}
path: junit/test-results-${{ matrix.python-version }}-${{ matrix.os }}.xml
# Use always() to always run this step to publish test results when there are test failures
if: ${{ always() }}
.github/workflows/python-tests.yml:45
- The
github-checkPyright reporter needs permission to create check runs, but the rewrite removes the job-levelchecks: writepermission that the previous Pyright job declared. On repositories with read-only default tokens, this check publication will fail or be silently unavailable; restore the required permission for the build job.
- name: Run Pyright
uses: jordemort/action-pyright@v1
with:
github_token: ${{ secrets.GITHUB_TOKEN }}
reporter: github-check
lib:
level: warning
filter_mode: file
CODEOWNERS:1
- Replacing the maintainer-only CODEOWNERS entries with one wildcard grants
@ARKlab/team-teapot418ownership of.github/,CODEOWNERS,LICENSE, andrenovate.json, which were previously restricted to@ARKlab/maintainers. This weakens review protection for workflow and ownership changes; preserve the specific maintainer rules.
* @ARKlab/team-teapot418 @ARKlab/maintainers
README.md:779
- This code block contains Python syntax, but the
csharpfence causes incorrect syntax highlighting and documentation rendering.
```csharp
README.md:884
- The table header misspells
REPLACE, making the documented upsert mode inconsistent with the API name.
| DATETIME | EXISTING | PAYLOAD | MERGE | REPALACE |
src/Artesian/GMEPublicOffers/_Enum/Status.py:13
- Removing
Status.DISfrom the exported enum breaks existing callers and prevents querying that status;GMEPublicOfferQueryno longer maps it. This unrelated public API removal needs explicit compatibility or versioning.
PREJ = 9
src/Artesian/MarketData/MarketDataService.py:537
- Deleting both
derivedTransformQueryValidationmethods removes a public sync/async API and makes existing consumers fail withAttributeError. This is outside the stated performance change; retain compatibility or explicitly document and version the removal.
src/Artesian/MarketData/_Dto/DerivedCfg.py:21 - Removing
transformfrom exportedDerivedCfgbreaks existing callers at construction time (DerivedCfg(..., transform=...)now raisesTypeError), independent of the service implementation. Preserve the field with a deprecation path or explicitly ship the breaking API change.
orderedReferencedMarketDataIds: Optional[List[int]]
src/Artesian/MarketData/_Enum/DerivedAlgorithm.py:7
- Removing
Transformfrom this exported enum is a breaking API change: existing clients referencingDerivedAlgorithm.Transformnow fail before serialization. The PR description only covers serializer performance; retain the member or make this an explicitly versioned and documented breaking change.
Coalesce = 2
src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py:64
- Before this optimization, unsupported mapping keys went through
jsons.dump(..., use_enum_name=True); converting them withstrchanges the wire format and can break deserialization (for example, enum keys lose their configured name and tuple/custom keys become repr strings). Keep the primitive/datetime fast paths, but preserve the jsons fallback for other keys.
k_out = str(k)
- Files reviewed: 21/22 changed files
- Comments generated: 2
- Review effort level: Lite
| coverage-report: | ||
| name: "Code Coverage" | ||
| publish: | ||
| if: github.event_name == 'push' && (github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/tags/v')) |
| @@ -25,7 +25,3 @@ class Market(Enum): | |||
| MIXBID = 70 | |||
| AFRR = 100 | |||
| AFRE = 200 | |||
feat(pythonperformance): Improved performance of __artesianDictSerializer