Skip to content

Improved performance of write big TimeSerie - #64

Open
jacopocinaark wants to merge 11 commits into
masterfrom
feature/22400-PythonPerformance
Open

jacopocinaark wants to merge 11 commits into
masterfrom
feature/22400-PythonPerformance

Conversation

@jacopocinaark

Copy link
Copy Markdown
Contributor

feat(pythonperformance): Improved performance of __artesianDictSerializer

@jacopocinaark
jacopocinaark requested review from a team as code owners June 26, 2026 12:09
Copilot AI lite review requested due to automatic review settings September 16, 2026 07:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 isinstance fast path also matches IntEnum/StrEnum values, so they bypass jsons.dump and its configured use_enum_name=True behavior. 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.

Comment thread src/Artesian/_ClientsExecutor/ArtesianJsonSerializer.py
…ARKlab/Artesian.SDK-Python into feature/22400-PythonPerformance

 It looks like you may be committing a merge.
Copilot AI review requested due to automatic review settings September 16, 2026 08:27
Comment thread CODEOWNERS Dismissed
Comment thread CODEOWNERS

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the original CODEOWNER is correct. LICENSE, CODEOWNERS, renovate.json and .github/ configuration (workflows, checks, and other .github setup) MUST be vetted by ARKlab/maintainers

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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 v tag 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 honors use_enum_name and registered serializers. Converting arbitrary keys with str changes the wire value—for example an enum key becomes Market.MGP instead of MGP—and can make deserialization fail. Keep the primitive fast path, but retain jsons.dump for the fallback.
            k_out = str(k)
  • Files reviewed: 21/22 changed files
  • Comments generated: 11
  • Review effort level: Lite

Comment thread .github/workflows/python-tests.yml
Comment thread .github/workflows/python-tests.yml
Comment thread CODEOWNERS
Comment thread src/Artesian/GMEPublicOffers/_Enum/Market.py
Comment thread src/Artesian/GMEPublicOffers/_Enum/Status.py
Comment thread src/Artesian/MarketData/_Dto/DerivedCfg.py
Comment thread src/Artesian/MarketData/_Enum/DerivedAlgorithm.py
Comment thread .github/workflows/python-tests.yml
Comment thread README.md
Comment thread README.md
Copilot AI review requested due to automatic review settings September 16, 2026 08:44
@jacopocinaark
jacopocinaark force-pushed the feature/22400-PythonPerformance branch from c71dff1 to c529b1b Compare September 16, 2026 08:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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 jsons serializer for non-primitive keys. For example, with use_enum_name=True, an enum key that previously serialized to its member name is now converted by str() to EnumClass.Member, and custom key serializers are bypassed; retain the jsons.dump fallback 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

Copilot AI review requested due to automatic review settings September 16, 2026 08:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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 configured use_enum_name=True, the previous jsons.dump(k, key_transformer=None, **kwargs) serialized an enum key as ActualTimeSerie, while this emits MarketDataType.ActualTimeSerie; custom key serializers are also bypassed. Keep the jsons.dump fallback 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

Copilot AI review requested due to automatic review settings September 16, 2026 08:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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 tags filters use glob patterns, not regex. The + characters and escaped dots make this pattern fail to match a tag such as v1.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, and renovate.json with the broad * rule, allowing team-teapot418 to 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; use REPLACE to 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 Market members makes existing queries for GMP_ASTA, GMP_Cont, MI_Cont, and MGS fail 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.DIS breaks 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 through jsons.dump (for example, Status.ACC becomes ACC), but this now becomes Status.ACC; registered serializers for other key types are also bypassed. Keep the jsons.dump fallback for non-primitive keys.
            k_out = str(k)
  • Files reviewed: 21/22 changed files
  • Comments generated: 5
  • Review effort level: Lite

Comment thread .github/workflows/python-tests.yml
Comment thread src/Artesian/MarketData/MarketDataService.py

Attributes:
derivedAlgorithm: the derived configuration algorithm (MUV, Coalesce, Sum, Transform)
derivedAlgorithm: the derived configuration algorithm (MUV, Coalesce, Sum)
Comment thread src/Artesian/MarketData/_Enum/DerivedAlgorithm.py
Comment thread .github/workflows/python-tests.yml
Copilot AI review requested due to automatic review settings September 16, 2026 09:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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 str bypasses the serializer settings. With use_enum_name=True configured below, an Enum key previously serialized through jsons.dump as its member name, but now becomes a string such as MarketDataType.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

  • isinstance also matches scalar subclasses such as IntEnum and str-backed enums, so these values now bypass jsons.dump and its use_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

Copilot AI review requested due to automatic review settings September 16, 2026 09:26
Comment thread CODEOWNERS
/LICENSE @ARKlab/maintainers
/CODEOWNERS @ARKlab/maintainers
/renovate.json @ARKlab/maintainers
* @ARKlab/team-teapot418 @ARKlab/maintainers

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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.tags patterns use GitHub glob syntax, not regular expressions. [0-9]+ therefore requires a literal +, so a normal tag such as v1.2.3 does 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.xml and 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-check Pyright reporter needs permission to create check runs, but the rewrite removes the job-level checks: write permission 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-teapot418 ownership of .github/, CODEOWNERS, LICENSE, and renovate.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 csharp fence 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.DIS from the exported enum breaks existing callers and prevents querying that status; GMEPublicOfferQuery no longer maps it. This unrelated public API removal needs explicit compatibility or versioning.
    PREJ = 9

src/Artesian/MarketData/MarketDataService.py:537

  • Deleting both derivedTransformQueryValidation methods removes a public sync/async API and makes existing consumers fail with AttributeError. This is outside the stated performance change; retain compatibility or explicitly document and version the removal.
    src/Artesian/MarketData/_Dto/DerivedCfg.py:21
  • Removing transform from exported DerivedCfg breaks existing callers at construction time (DerivedCfg(..., transform=...) now raises TypeError), 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 Transform from this exported enum is a breaking API change: existing clients referencing DerivedAlgorithm.Transform now 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 with str changes 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
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.

5 participants