Conversation
WalkthroughMigrates code and imports from the Changes
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go-sdk/pkg/clients/numerix/v1.go (1)
171-180:⚠️ Potential issue | 🟠 MajorPotential nil pointer dereference.
fillResponseFromBatchcallsbatchProtoResp.GetError()without first checking ifbatchProtoRespis nil. Ifresult.responseis nil (which can happen when an error occurs in batch processing per line 147), calling this method will panic.🐛 Proposed fix
func (c *ClientV1) fillResponseFromBatch(finalResponse *NumerixResponse, batchProtoResp *grpc.NumerixResponseProto, batch BatchInfo) { + if batchProtoResp == nil { + return + } if errProto := batchProtoResp.GetError(); errProto != nil { log.Warn().Msgf("Received error in proto response error: %s", errProto.GetMessage()) return }
🧹 Nitpick comments (4)
go-sdk/go.mod (1)
6-16: All dependency versions exist and resolve correctly; however, several have newer releases available.All listed versions are valid and resolvable. Multiple modules have available updates:
go.etcd.io/etcd/client/v3: v3.6.8 available (current: v3.5.17)google.golang.org/grpc: v1.79.1 available (current: v1.68.2)golang.org/x/net: v0.50.0 available (current: v0.42.0)google.golang.org/protobuf: v1.36.11 available (current: v1.36.9)Note that
github.com/x448/float16(v0.8.4, released 2020-01-17) andgithub.com/soheilhy/cmux(v0.1.5, released 2021-02-05) are notably outdated. Consider evaluating whether these should be updated or if their pinned versions are intentional.go-sdk/pkg/clients/numerix/v1.go (2)
34-61: Potential race condition in singleton initialization.The check
if client == nilon line 35 is outside thesync.Onceblock, which could lead to a race condition. If two goroutines callInitV1Clientconcurrently and both seeclient == nil, only one will execute theonce.Doblock, but this pattern is redundant and potentially confusing.The
sync.Oncealready guarantees single execution, so the outer nil check is unnecessary.♻️ Suggested simplification
func InitV1Client(configBytes []byte) NumerixClient { - if client == nil { - once.Do(func() { - byteorder.Init() + once.Do(func() { + byteorder.Init() - clientConfig, err := getClientConfigs(configBytes) - if err != nil { - log.Panic().Err(err).Msgf("Invalid numerix client configs: %#v", clientConfig) - } - headers = metadata.New(map[string]string{ - numerix_CALLER_ID: clientConfig.CallerId, - }) + clientConfig, err := getClientConfigs(configBytes) + if err != nil { + log.Panic().Err(err).Msgf("Invalid numerix client configs: %#v", clientConfig) + } + headers = metadata.New(map[string]string{ + numerix_CALLER_ID: clientConfig.CallerId, + }) - grpcClient, grpcErr := getGrpcClient(clientConfig) - if grpcErr != nil { - log.Panic().Err(grpcErr).Msgf("Error creating numerix service grpc client, client: %#v", grpcClient) - } + grpcClient, grpcErr := getGrpcClient(clientConfig) + if grpcErr != nil { + log.Panic().Err(grpcErr).Msgf("Error creating numerix service grpc client, client: %#v", grpcClient) + } - numerixClient := grpc.NewNumerixClient(grpcClient) - client = &ClientV1{ - ClientConfigs: clientConfig, - GrpcClient: grpcClient, - numerixClient: numerixClient, - Adapter: Adapter{}, - } - }) - } + numerixClient := grpc.NewNumerixClient(grpcClient) + client = &ClientV1{ + ClientConfigs: clientConfig, + GrpcClient: grpcClient, + numerixClient: numerixClient, + Adapter: Adapter{}, + } + }) return client }
160-166: Silent continuation on batch errors may cause incomplete results.When a batch fails (line 161-163), the code only logs a warning and continues. The final response will have
nilentries at positions corresponding to failed batches sincefillResponseFromBatchreturns early for nil responses. This may be intentional for partial success, but callers should be aware thatfinalResponse.ComputationScoreData.Datamay contain nil entries.Consider either:
- Documenting this behavior
- Returning an error if any batch fails
- Adding a mechanism to indicate partial failure to callers
go-sdk/pkg/clients/numerix/adaptor.go (1)
38-45: Error inScoreDataToFP32Bytessilently returns without indication.When
typeconverter.ConvertBytesToBytesfails, the method logs the error and returns early, leavingrequest.EntityScoreData.Datain a partially converted state. The callerMapRequestToProtohas no way to know the conversion failed.This is pre-existing behavior, but worth noting: consider returning an error or using a different pattern to signal failures.
removed helix client
2 participants
🔁 Pull Request Template – BharatMLStack
Context:
Give a brief overview of the motivation behind this change. Include any relevant discussion links (Slack, documents, tickets, etc.) that help reviewers understand the background and the issue being addressed.
Describe your changes:
Mention the changes made in the codebase.
Testing:
Please describe how you tested the code. If manual tests were performed - please explain how. If automatic tests were added or existing ones cover the change - please explain how did you run them.
Monitoring:
Explain how this change will be tracked after deployment. Indicate whether current dashboards, alerts, and logs are enough, or if additional instrumentation is required.
Rollback plan
Explain rollback plan in case of issues.
Checklist before requesting a review
📂 Modules Affected
horizon(Real-time systems / networking)online-feature-store(Feature serving infra)trufflebox-ui(Admin panel / UI)infra(Docker, CI/CD, GCP/AWS setup)docs(Documentation updates)___________✅ Type of Change
___________📊 Benchmark / Metrics (if applicable)
Summary by CodeRabbit
Chores
Infrastructure
Documentation