feat: concurrency decorator#3089
Conversation
7ce91e8 to
e433b85
Compare
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds the @cog.concurrent decorator for configuring async predictor concurrency, with static extraction at build time and runtime enforcement. The decorator integrates cleanly with the existing precedence chain (runtime env > YAML > decorator > default) and properly deprecates the concurrency.max YAML field.
Issues found:
- Missing parser test for qualified alias import pattern — The tree-sitter parser tests cover direct import, qualified import (
import cog), and imported alias (from cog import concurrent as foo), but lack coverage for the qualified alias pattern (import cog as c+@c.concurrent). The streaming decorator has this test (TestStreamingDecoratorQualifiedAliasOptIn) and the concurrent implementation uses the sameattributeIsCogConcurrenthelper, so it likely works — but explicit coverage prevents regressions.
|
I reviewed PR #3089 (add Finding: Missing parser test for Everything else looked solid — the decorator implementation, build-time static extraction, precedence handling, runtime enforcement in Coglet, and deprecation of YAML concurrency are all correctly implemented. |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds the @cog.concurrent decorator for configuring async predictor concurrency, with static extraction at build time and runtime enforcement. Overall the implementation is solid — the decorator, precedence handling, runtime checks in Coglet, and deprecation of YAML concurrency are all correctly implemented.
Issues found:
- Missing parser test for qualified alias import pattern — The previous review already flagged this, but the test still hasn't been added. The tree-sitter parser tests cover direct import, qualified import (
import cog), and imported alias (from cog import concurrent as foo), but lack coverage for the qualified alias pattern (import cog as c+@c.concurrent). The streaming decorator has this test (TestStreamingDecoratorQualifiedAliasOptIn) and the concurrent implementation uses the sameattributeIsCogConcurrenthelper, so it likely works — but explicit coverage prevents regressions. - CodeQL integer conversion warning — The
int64→intconversion atpkg/schema/python/annotations.go:309triggered a CodeQL alert. The code does have an upper bound check at line 306, butint64(math.MaxInt)may not be recognized as a proper bound by static analysis. Extracting the bound to a named constant silences the warning without changing behavior.
|
Posted a batch review to PR #3089 with two suggestions:
|
Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com> Signed-off-by: Anish Sahoo <anishsahoo2005@gmail.com>
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds the @cog.concurrent decorator for configuring async predictor concurrency, with static extraction at build time and runtime enforcement. Overall the implementation is solid — the decorator, precedence handling, runtime checks in Coglet, and deprecation of YAML concurrency are all correctly implemented.
Issues found:
- Go syntax error in parser tests — There's a duplicate empty function definition at line 1053 that will cause a compilation error.
- CodeQL integer conversion warning — The
int64→intconversion atpkg/schema/python/annotations.go:306still triggers a CodeQL alert. Extracting the bound to a named constant silences the warning without changing behavior.
Merge from #3089 introduced a duplicate TestConcurrentDecoratorIgnoredWhenNotFromCog function signature that caused a syntax error.
|
Posted a batch review to PR #3089 with two issues:
|
|
LGTM |
Summary
Example
Testing
Fixes #2811