feat(sdk): monorepo repository to add new otel plugin package#397
Conversation
SilanHe
force-pushed
the
feat/add-otel-package
branch
from
3b74d87 to
7bded69
Compare
There was a problem hiding this comment.
I haven't actually tested this one but it looks quite reasonable. I had to work a little more closely with AI on this one.
|
linting and tests failed. |
Ah sorry, I was still working through the different worklows, there was an issue with a workflow which would check out the testing sdk into the root. Then the root testing scripts would pick up the testing sdk pyproject.toml |
There was a problem hiding this comment.
still have to do a full review, but worried about a few things
-
${{ github.event.release.target_commitish }} is the branch the release was created from (typically main). With this ref:, the build will check out the branch's HEAD at workflow run time rather than the tagged commit. That's a problem if main has moved on between when the operator pressed "Publish release" and when the workflow actually runs (concurrent merges, multi-package release windows): we'd ship whatever is on main, not the code at the tag.
-
test:cov lost --cov-fail-under=98; CI no longer enforces 98
-
.gitignore out ot date with new changes
-
CONTRIBUTING guide out of data with new changes
|
out of date changes because this PR is open for too long. It's better to merge now than later and we can add the identified missing changes later. Otherwise we'll see more missing changes. |
woops sorry, I meant to pin it to the tag but picked the wrong variable. I had just noticed that the workflow never did that in the past so I figured I would fix it. Thanks for pointing this out, going to use
spoke offline, we'll follow up
updated gitignore to use * instead of referencing the path directly
use package name instead of examples |
|
|
||
| /examples/build/* | ||
| /examples/*.zip | ||
| */build/* |
There was a problem hiding this comment.
not blocking, but fix in follow-up pls:
*/build/* doesn't match packages/<pkg>/build/<file> because * doesn't span / (gitignore(5)). Either **/build/* + **/*.zip, or (preferred — scoped to packages/) packages/*/build/ + packages/*/*.zip.
| assert isinstance(parent_call[1]["value"], BatchResult) | ||
| finally: | ||
| importlib.reload(child) | ||
| assert parent_call[1]["value"] is result |
There was a problem hiding this comment.
This assert ... is result is in the finally: clause (after importlib.reload(child)), not inside the try:/with patch(...) body. Asymmetric to parallel_test.py L1140 where the equivalent line is correctly inside the with block. As-is, if the try-body raises before parent_call binds, the finally raises UnboundLocalError and masks the original exception. Could you move this line back inside the with block (3-line indent fix)?
|
|
||
| ```python | ||
| from aws_durable_execution_sdk_python import DurableContext, durable_execution | ||
| from aws_durable_execution_sdk_python_otel import instrument_durable_execution |
There was a problem hiding this comment.
not blockling: these don't match the code ,__init__.py only re-exports __version__, instrument_durable_execution` doesn't exist yet, and the Features section below documents capabilities (auto span creation, replay-aware tracing, error recording) that aren't implemented.
maybe add something like DRAFT 'v0.1.0 reserves the package name; instrumentation lands in v0.2.0'.
| @@ -1,18 +1,62 @@ | |||
|
|
|||
| #!/bin/sh | |||
There was a problem hiding this comment.
Not blocking: Shebang is sh #!/bin/sh but lines 39 and 45 use bash array syntax (PACKAGES=( ... ), "${PACKAGES[@]}").
Will fail under dash (the default /bin/sh on Debian/Ubuntu).
either amend sheabing to #!/usr/bin/env bash, or rewrite to sh compliant.
| "SIM117", | ||
| "TRY301", | ||
| ] | ||
|
|
There was a problem hiding this comment.
There are some warnings in the newest integration test CI workflow
packages/aws-durable-execution-sdk-python-examples/test/step/test_step.py:10
/home/runner/work/aws-durable-execution-sdk-python/aws-durable-execution-sdk-python/language-sdk/packages/aws-durable-execution-sdk-python-examples/test/step/test_step.py:10: PytestUnknownMarkWarning: Unknown pytest.mark.example - is this a typo? You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
@pytest.mark.exampleIs this something caused by the mono repo change? There was no warning before (e.g. test run history) - @wangyb-A
hmm so I deleted these but moved them to the aws-durable-execution-sdk-python-examples pyproject.toml.
I think since I'm running the tests from the root, it's not reading the pyproject.toml from the package itself
4 participants
Issue #, if available:
Description of changes:
Monorepo Restructure
The repo was converted from a single-package layout to a monorepo with a
packages/directory:src/,tests/,examples/,pyproject.toml) intopackages/aws-durable-execution-sdk-python/New Package: OpenTelemetry Instrumentation
A new package was added at
packages/aws-durable-execution-sdk-python-otel/(v0.1.0):CI & Workflow Updates
ci.yml— Loops over allpackages/*/directories for format checks, type checking, tests, and buildsci-checks.sh— Iterates over aPACKAGESarray instead of running checks at the repo rootpypi-publish.yml— Uses a matrix strategy to build and publish each package independently; per-package artifact namessync-package.yml— Deleted (no longer needed)deploy-examples.yml/integration-tests.yml— Minor path adjustmentsDocumentation
CONTRIBUTING.mdcdinto package dirs before running hatchRELEASING.md(new)sdk-vX.Y.Z,otel-vX.Y.Z), PyPI publishing flow, release notes format, pre-publish checklistREADME.md(new)Misc
.gitignore— Relaxed example build/zip patterns from root-anchored to relative (works with nested layout)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.