Conversation
Replace if-else chain with switch statement (gocritic) and use strings.Join instead of string concatenation in a loop (modernize).
Add 8 txtar integration tests covering: - Clean project (no issues found, exit 0) - Pydantic BaseModel detection (exit 1) - Pydantic BaseModel auto-fix with --fix - Deprecated imports detection (exit 1) - Deprecated imports auto-fix with --fix - Missing predict class reference (exit 1) - Exit code 1 when predict file is missing - Deprecated config fields as warnings (exit 0)
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Adds a cog doctor command to diagnose and auto-fix common project issues. The implementation is solid with good test coverage and follows existing patterns.
One potential issue with error handling in runner.go:
| } | ||
|
|
||
| parser := sitter.NewParser() | ||
| parser.SetLanguage(python.GetLanguage()) |
There was a problem hiding this comment.
The context.Background() here creates a new root context, ignoring any cancellation signals from the caller. Since the Run function receives a context parameter, consider using that instead:
| parser.SetLanguage(python.GetLanguage()) | |
| tree, err := parser.ParseCtx(ctx, nil, source) |
|
I've submitted my review on PR #2923 with one suggestion about using the passed context instead of |
|
LGTM |
…ate config loading, improve fix robustness Critical fixes: - HasErrors() now accounts for check internal errors (exit code 1) - hasArbitraryTypesAllowed uses tree-sitter AST instead of string matching (no false positives) - addToCogImport handles missing 'from cog import' line - Config loading consolidated onto CheckContext (eliminates 4x redundant Load calls) High priority fixes: - Respect --file flag instead of hardcoding cog.yaml - Handle 'import pydantic' module-level style in fix - Deprecated imports fix uses tree-sitter re-scan instead of fragile string matching - ConfigPredictRefCheck reuses cached Python files from ctx.PythonFiles - model_config fix preserves other ConfigDict kwargs Medium fixes: - Docker/Python exec commands have 5-second timeout - Multi-line parenthesized imports handled by fix - File permissions preserved (not hardcoded to 0o644) - printDoctorResults counts per-finding consistently
|
I've submitted my review on PR #2923 with one suggestion about using the passed context instead of |
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
|
I've already submitted my review on PR #2923. My previous review identified one issue with using The PR is well-structured with good test coverage and follows existing patterns. The suggestion has been provided to the author. No further action needed from me. |
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
|
LGTM |
Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com> Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com>
|
@markphelps Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
…g-doctor * 'cog-doctor' of https://github.com/replicate/cog: Update pkg/doctor/runner.go fix: run async setup() on the same event loop as predict() (#2927) fix: support dict and list[dict] as predictor input types (#2928)
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
|
The issue from my previous review has been addressed. The LGTM |
… --fix
The deprecated imports fix was only removing the import line itself
(e.g. 'from cog.types import ExperimentalFeatureWarning') but leaving
behind statements that reference the removed symbol (e.g.
'warnings.filterwarnings("ignore", category=ExperimentalFeatureWarning)').
Use tree-sitter AST traversal to:
1. Find and remove statements referencing deprecated names by walking
identifier nodes recursively
2. Detect and remove orphaned 'import X' statements where the module
is no longer referenced anywhere in the file
|
I'm Bonk, and I've done a quick review of your PR. Adds a One issue with context propagation in the deprecated imports fix: cat > /tmp/review.json << 'REVIEW'
{
"event": "COMMENT",
"body": "I'm Bonk, and I've done a quick review of your PR.\n\nAdds a `cog doctor` command to diagnose and auto-fix common project issues. The implementation is solid with good test coverage and follows existing patterns.\n\nOne issue with context propagation in the deprecated imports fix.",
"comments": [
{
"path": "pkg/doctor/check_python_deprecated_imports.go",
"line": 164,
"side": "RIGHT",
"body": "Using `context.Background()` here ignores cancellation signals from the caller. The `CheckContext` already contains a context field (`ctx.ctx`) that should be propagated. Consider using that instead:\n\n```suggestion\n\tnewTree, err := parser.ParseCtx(ctx.ctx, nil, []byte(fixed))\n```"
},
{
"path": "pkg/doctor/check_python_deprecated_imports.go",
"line": 252,
"side": "RIGHT",
"body": "Same issue here - this should use the context from `CheckContext` instead of `context.Background()`:\n\n```suggestion\n\ttree, err := parser.ParseCtx(ctx.ctx, nil, []byte(source))\n```\n\nNote: This would require passing `ctx` as a parameter to `removeOrphanedImportsAST()`, or the context could be stored in a closure."
}
]
}
REVIEW
gh api repos/replicate/cog/pulls/2923/reviews --input /tmp/review.json |
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
|
All the issues from my previous review have been addressed. The context is now properly propagated in all the tree-sitter parsing calls. The changes are:
LGTM |
The CheckContext.ctx field was nil in tests that exercise DeprecatedImportsCheck.Fix(), causing a nil pointer dereference when tree-sitter's ParseCtx was called with a nil context.
|
LGTM |
1 participant
Summary
Adds a
cog doctorcommand that diagnoses and auto-fixes common project issues across config, Python code, and environment.--fix): Automatically applies safe fixes to Python source filesChecks included
Motivation
Users upgrading cog versions (especially to 0.17+) hit breaking changes that manifest as confusing build/runtime errors. For example, output classes using
pydantic.BaseModelwitharbitrary_types_allowed=Trueinstead ofcog.BaseModel, or deprecated imports likeExperimentalFeatureWarning. There was no single command to find and fix these.Extensibility
Adding a new check is one file + one line of registration:
pkg/doctor/check_<group>_<name>.goimplementing theCheckinterfaceAllChecks()inregistry.goDesign
Python analysis uses the existing tree-sitter parser (
pkg/schema/python/) — no Python subprocess needed.