Dtype changes #481
Dtype changes #481
Conversation
* implement logging * add EOF initialise content in log_handler * use poetry lock to update * fix poetry missing dependencies * fix dependencies * reformatted * fix p fix duration add span_id * add temp act * log act * add noqa * reformat * add ignore * add d only in response and delight * use td from thread * add to delight log * run black * null as default values * remove header from delight
* Adding security workflow for onboarding WASP * Updated Hardcoded FR Project key as a variable for easy migration
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds OpenTelemetry instrumentation, structured logging, and dtype configuration to support the nomic-ai/nomic-embed-text-v1.5 model while enhancing observability.
- Added new security workflow
/.github/workflows/security.ymlimplementing Wasp (Semgrep) SAST scanning for code safety - Introduced structured logging in
/libs/infinity_emb/infinity_emb/log_handler.pywith OpenTelemetry integration for distributed tracing - Modified
/libs/infinity_emb/Dockerfileto support configurable dtype and base CUDA image via build arguments - Added
/libs/infinity_emb/environment_config.shfor OpenTelemetry configuration and service startup - Added einops dependency in
pyproject.tomlto support nomic-ai/nomic-embed-text-v1.5 model
7 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
| pull_request_target: | ||
| branches: | ||
| - main |
There was a problem hiding this comment.
logic: pull_request_target is a security risk as it runs with repository secrets on external PR code. Consider using pull_request instead if repository secrets are not needed, or add explicit ref validation
| group: security-lrg | ||
| steps: | ||
| - name: Setting permission | ||
| run: sudo chown runner:runner -R .* |
There was a problem hiding this comment.
logic: The wildcard pattern .* in chown is overly broad and could affect hidden files. Use a more specific path like '.'
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Running Wasp scan | ||
| uses: freshactions/wasp@latest |
There was a problem hiding this comment.
style: Using @latest tag for actions can be dangerous. Pin to a specific version for security and stability
| @@ -6,6 +6,10 @@ __pycache__/ | |||
| # C extensions | |||
| *.so | |||
|
|
|||
| # Pycharn | |||
There was a problem hiding this comment.
syntax: 'Pycharn' is misspelled, should be 'PyCharm'
| @@ -6,6 +6,10 @@ __pycache__/ | |||
| # C extensions | |||
| *.so | |||
|
|
|||
| # Pycharn | |||
| .idea | |||
| models/ | |||
There was a problem hiding this comment.
logic: Ignoring 'models/' directory may prevent tracking of important model files that should be version controlled. Consider being more specific about which model files to ignore
| #export OTEL_TRACES_SAMPLER=parentbased_always_off | ||
| export OTEL_RESOURCE_ATTRIBUTES=service.name=${SHERLOCK_SERVICE_NAME},host.name=${POD_NAME},host.ip=${POD_IP} | ||
| export OTEL_EXPORTER_OTLP_ENDPOINT=http://${HOST_IP}:5680 | ||
| infinity_emb v2 --model-id /models --dtype ${DTYPE} |
There was a problem hiding this comment.
style: The hardcoded /models path assumes a specific directory structure. Consider making this configurable via environment variable
| @@ -129,6 +133,7 @@ async def validate_token( | |||
|
|
|||
| instrumentator = Instrumentator().instrument(app) | |||
| app.add_exception_handler(errors.OpenAIException, errors.openai_exception_handler) | |||
| app.add_middleware(StructuredLoggingMiddleware) | |||
There was a problem hiding this comment.
logic: StructuredLoggingMiddleware should be added before other middleware like CORSMiddleware to ensure all requests are properly logged
| extra_index = 9 # obtained from function signature, see above comment. | ||
| extra = args[extra_index] |
There was a problem hiding this comment.
logic: Hardcoding the extra_index is fragile and could break with Python/logging updates. Consider using inspect module to find index dynamically
| def nice_format(data): | ||
| if type(data) in (int, str, float): | ||
| return data | ||
| try: | ||
| return json.dumps(data, ensure_ascii=False) | ||
| except Exception: | ||
| return str(data) |
There was a problem hiding this comment.
style: Using type() instead of isinstance() for type checking is not recommended as it breaks inheritance
| opentelemetry-instrumentation-logging = "^0.48b0" | ||
| opentelemetry-sdk = "^1.27.0" |
There was a problem hiding this comment.
logic: OpenTelemetry dependencies are added as required but should likely be optional since they are only needed for logging instrumentation
Related Issue
Checklist
Additional Notes
Add any other context about the PR here.