Skip to content

feature: add logging in orchestrator #128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

yuejiaointel
Copy link
Contributor

@yuejiaointel yuejiaointel commented May 21, 2025

Follow up PR on logging
Target:
Adding logging in
orchestrator level:
Functions to add:
experimental_calibrate
search
consolidate
add_points
delete_points/entries

index level:
compact

@yuejiaointel yuejiaointel changed the title feature: add logging in orchestrator: experimental_calibrate feature: add logging in orchestrator May 27, 2025
@yuejiaointel yuejiaointel marked this pull request as ready for review May 28, 2025 00:18
@napetrov napetrov requested review from mihaic, ibhati and Copilot and removed request for mihaic and ibhati June 18, 2025 17:37
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request adds a logger parameter to multiple functions within the orchestrator and index modules to enable detailed logging of operations such as calibration, search, consolidation, point addition, and deletion. Key changes include propagating the logger argument through function calls, updating test calls to include logger parameters, and ensuring fallback behavior for logger usage is maintained.

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/svs/index/vamana/dynamic_index_2.cpp Updated test_loop to pass logger to add/delete calls
tests/integration/vamana/index_search.cpp Propagated logger and introduced log verification checks
include/svs/orchestrators/vamana.h Added logger parameter to orchestrator APIs
include/svs/orchestrators/manager.h Updated manager interface to support logger
include/svs/orchestrators/dynamic_vamana.h Added logger parameter to dynamic operations
... (other files updated similarly) Logger parameter added and propagated throughout
Comments suppressed due to low confidence (2)

include/svs/orchestrators/vamana.h:95

  • Consider updating the inline documentation for the function signature to include information about the new logger parameter and its default behavior.
        const index::vamana::CalibrationParameters& calibration_parameters,

include/svs/index/vamana/dynamic_index.h:707

  • It would be beneficial to document the fallback logic (using logger_ if available, otherwise logger) so that users understand how the logger is determined in this context.
        );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant