Layers -18 Label detection tests#1912
Open
bhanvimenghani wants to merge 10 commits into
Open
Conversation
Contributor
Reviewer's GuideAdds full CRUD support and richer validation for layer REST APIs, updates DB DAO and metrics, refactors success/error responses to JSON, and significantly expands test coverage (including label-based detection, validation, and performance) while replacing direct DB cleanup with DELETE API usage. Sequence diagram for updateLayer REST API flowsequenceDiagram
actor Client
participant LayerService
participant Converters
participant LayerValidation
participant ExperimentDAOImpl
participant HibernateSession
participant Database
Client->>LayerService: HTTP PUT /updateLayer?name={layerName}
LayerService->>LayerService: validate layerName and query params
LayerService->>LayerService: read request body
LayerService->>Converters: convertInputJSONToCreateLayer(inputData)
Converters-->>LayerService: KruizeLayer or IllegalArgumentException
alt invalid JSON or null elements
LayerService-->>Client: 400 JSON error response (UpdateLayerAPI.VALIDATION_FAILED)
else valid KruizeLayer
LayerService->>LayerService: compare URL layerName with kruizeLayer.getLayerName()
alt name mismatch
LayerService-->>Client: 400 JSON error response (LAYER_NAME_MISMATCH)
else names match
LayerService->>LayerValidation: validate(kruizeLayer)
LayerValidation-->>LayerService: ValidationOutputData
alt validation failed
LayerService-->>Client: 400 JSON error response (VALIDATION_FAILED)
else validation passed
LayerService->>ExperimentDAOImpl: loadLayerByName(kruizeLayer.getLayerName())
ExperimentDAOImpl->>HibernateSession: openSession()
HibernateSession->>Database: SELECT layer by name
Database-->>HibernateSession: existingLayers
HibernateSession-->>ExperimentDAOImpl: existingLayers
ExperimentDAOImpl-->>LayerService: List<KruizeLMLayerEntry>
alt no existingLayers
LayerService-->>Client: 404 JSON error response (LAYER_NOT_FOUND)
else layer exists
LayerService->>LayerService: convertToLayerEntry(kruizeLayer)
LayerService->>ExperimentDAOImpl: updateLayerToDB(KruizeLMLayerEntry)
ExperimentDAOImpl->>HibernateSession: openSession and beginTransaction()
HibernateSession->>Database: merge KruizeLMLayerEntry
Database-->>HibernateSession: update result
HibernateSession-->>ExperimentDAOImpl: commitTransaction()
ExperimentDAOImpl-->>LayerService: ValidationOutputData
alt update success
LayerService-->>Client: 200 JSON success response (UPDATE_LAYER_SUCCESS_MSG)
else update failure
LayerService-->>Client: 500 JSON error response (UPDATE_LAYER_TO_DB_FAILURE)
end
end
end
end
end
Sequence diagram for deleteLayer REST API flowsequenceDiagram
actor Client
participant LayerService
participant ExperimentDAOImpl
participant HibernateSession
participant Database
Client->>LayerService: HTTP DELETE /deleteLayer?name={layerName}
LayerService->>LayerService: validate layerName and query params
alt invalid name or query params
LayerService-->>Client: 400 JSON error response (DeleteLayerAPI)
else valid params
LayerService->>ExperimentDAOImpl: loadLayerByName(layerName)
ExperimentDAOImpl->>HibernateSession: openSession()
HibernateSession->>Database: SELECT layer by name
Database-->>HibernateSession: existingLayers
HibernateSession-->>ExperimentDAOImpl: existingLayers
ExperimentDAOImpl-->>LayerService: List<KruizeLMLayerEntry>
alt no existingLayers
LayerService-->>Client: 404 JSON error response (DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME)
else layer exists
LayerService->>ExperimentDAOImpl: deleteLayerByName(layerName)
ExperimentDAOImpl->>HibernateSession: openSession and beginTransaction()
HibernateSession->>Database: DELETE layer by name
Database-->>HibernateSession: deletedCount
HibernateSession-->>ExperimentDAOImpl: commitTransaction()
ExperimentDAOImpl-->>LayerService: ValidationOutputData
alt delete success
LayerService-->>Client: 200 JSON success response (DELETE_LAYER_SUCCESS_MSG)
else delete failure
LayerService-->>Client: 500 JSON error response (DeleteLayerAPI.DELETE_LAYER_ENTRY_ERROR_MSG)
end
end
end
opt unexpected exceptions
LayerService-->>Client: 500 JSON error response (DeleteLayerAPI.UNEXPECTED_ERROR)
end
Updated class diagram for LayerService CRUD and related componentsclassDiagram
class LayerService {
+doPost(HttpServletRequest request, HttpServletResponse response) void
+doGet(HttpServletRequest request, HttpServletResponse response) void
+doPut(HttpServletRequest request, HttpServletResponse response) void
+doDelete(HttpServletRequest request, HttpServletResponse response) void
-sendSuccessResponse(HttpServletResponse response, String message, int httpStatusCode) void
-sendErrorResponse(HttpServletResponse response, Exception e, int httpStatusCode, String errorMsg) void
-sendJsonResponse(HttpServletResponse response, String message, int statusCode, String status) void
-convertToLayerEntry(KruizeLayer kruizeLayer) KruizeLMLayerEntry
}
class ExperimentDAO {
<<interface>>
+addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+updateLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+deleteLayerByName(String layerName) ValidationOutputData
+loadAllLayers() List~KruizeLMLayerEntry~
+loadLayerByName(String layerName) List~KruizeLMLayerEntry~
}
class ExperimentDAOImpl {
+addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+updateLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) ValidationOutputData
+deleteLayerByName(String layerName) ValidationOutputData
+loadAllLayers() List~KruizeLMLayerEntry~
+loadLayerByName(String layerName) List~KruizeLMLayerEntry~
}
class MetricsConfig {
+Timer timerAddLayerDB
+Timer timerLoadAllLayers
+Timer timerLoadLayerByName
+Timer timerUpdateLayerDB
+Timer timerDeleteLayerDB
+Timer_Builder timerBAddLayerDB
+Timer_Builder timerBLoadAllLayers
+Timer_Builder timerBLoadLayerByName
+Timer_Builder timerBUpdateLayerDB
+Timer_Builder timerBDeleteLayerDB
+meterRegistry() MeterRegistry
}
class Converters {
+convertInputJSONToCreateLayer(String inputData) KruizeLayer
}
class AnalyzerErrorConstants_UpdateLayerAPI {
+String INVALID_LAYER_NAME
+String LAYER_NOT_FOUND
+String INVALID_LAYER_JSON
+String UPDATE_LAYER_TO_DB_FAILURE
+String LAYER_NAME_MISMATCH
+String VALIDATION_FAILED
+String UNEXPECTED_ERROR
+String INVALID_QUERY_PARAMS
+String MISSING_REQUIRED_FIELD
}
class AnalyzerErrorConstants_DeleteLayerAPI {
+String DELETE_LAYER_ENTRY_NOT_FOUND_WITH_NAME
+String DELETE_LAYER_ENTRY_ERROR_MSG
+String INVALID_LAYER_NAME
+String UNEXPECTED_ERROR
+String INVALID_QUERY_PARAMS
}
class KruizeConstants_LayerAPIMessages {
+String CREATE_LAYER_SUCCESS_MSG
+String UPDATE_LAYER_SUCCESS_MSG
+String DELETE_LAYER_SUCCESS_MSG
+String VIEW_LAYERS_MSG
+String ADD_LAYER_TO_DB
+String UPDATE_LAYER_TO_DB
+String DELETE_LAYER_FROM_DB
+String LOAD_LAYER_FAILURE
+String LOAD_ALL_LAYERS_FAILURE
}
class KruizeSupportedTypes {
+Set~String~ CREATE_LAYERS_QUERY_PARAMS_SUPPORTED
+Set~String~ LIST_LAYERS_QUERY_PARAMS_SUPPORTED
+Set~String~ UPDATE_LAYERS_QUERY_PARAMS_SUPPORTED
+Set~String~ DELETE_LAYERS_QUERY_PARAMS_SUPPORTED
}
class AnalyzerConstants {
+String LAYER_NAME
}
class DBConstants_SQLQUERY {
+String SELECT_FROM_LAYER
+String SELECT_FROM_LAYER_BY_NAME
+String DELETE_FROM_LAYER_BY_NAME
}
LayerService ..> ExperimentDAO : uses
LayerService ..> Converters : uses
LayerService ..> KruizeSupportedTypes : uses
LayerService ..> AnalyzerErrorConstants_UpdateLayerAPI : uses
LayerService ..> AnalyzerErrorConstants_DeleteLayerAPI : uses
LayerService ..> KruizeConstants_LayerAPIMessages : uses
LayerService ..> AnalyzerConstants : uses
ExperimentDAOImpl ..|> ExperimentDAO
ExperimentDAOImpl ..> MetricsConfig : uses
ExperimentDAOImpl ..> DBConstants_SQLQUERY : uses
MetricsConfig ..> Timer : defines
AnalyzerErrorConstants_UpdateLayerAPI <-- AnalyzerErrorConstants_DeleteLayerAPI
KruizeSupportedTypes ..> AnalyzerConstants : parameter_name
DBConstants_SQLQUERY ..> KruizeLMLayerEntry : hibernate_entity
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In ExperimentDAOImpl.deleteLayerByName, consider instrumenting the DB call with a Timer similar to addLayerToDB/updateLayerToDB so delete operations are consistently tracked by metrics.
- The query-parameter validation logic in LayerService.doPut and doDelete is nearly identical; you might want to extract this into a small helper to avoid duplication and keep supported param handling in one place.
- The new delete_layer() helper in tests prints full responses for every delete, which may generate noisy logs in larger suites; consider gating these prints behind a verbosity flag or only logging on failure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In ExperimentDAOImpl.deleteLayerByName, consider instrumenting the DB call with a Timer similar to addLayerToDB/updateLayerToDB so delete operations are consistently tracked by metrics.
- The query-parameter validation logic in LayerService.doPut and doDelete is nearly identical; you might want to extract this into a small helper to avoid duplication and keep supported param handling in one place.
- The new delete_layer() helper in tests prints full responses for every delete, which may generate noisy logs in larger suites; consider gating these prints behind a verbosity flag or only logging on failure.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/services/LayerService.java" line_range="392" />
<code_context>
+ }
+
+ // Delete from database
+ ValidationOutputData deletedFromDB = experimentDAO.deleteLayerByName(layerName);
+
+ if (deletedFromDB.isSuccess()) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Distinguish between true server errors and not-found cases during delete to avoid returning 500 for a missing layer.
There’s a race between `loadLayerByName` and `deleteLayerByName`: if the layer is removed in between, `deletedFromDB.isSuccess()` will be false and we’ll return a 500 even though the message says "Layer not found" (should be 404). Consider making the DAO the single source of truth: either map its "not found" result to 404 here, or drop the pre-check and rely entirely on the delete call for existence and status mapping.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/database/dao/ExperimentDAOImpl.java" line_range="738-689" />
<code_context>
+ public ValidationOutputData deleteLayerByName(String layerName) {
</code_context>
<issue_to_address>
**suggestion:** Consider adding metrics timing for `deleteLayerByName` similar to other DB operations.
`MetricsConfig` already defines `timerBDeleteLayerDB` / `timerDeleteLayerDB`, and `updateLayerToDB` uses a `Timer.Sample`. `deleteLayerByName` currently doesn’t record timing; consider wrapping it in a `Timer.Sample` with success/failure tags for consistent observability with other DB operations.
Suggested implementation:
```java
@Override
public ValidationOutputData deleteLayerByName(String layerName) {
Timer.Sample sample = Timer.start(MetricsConfig.meterRegistry);
ValidationOutputData validationOutputData = new ValidationOutputData(false, null, null);
Transaction tx = null;
try (Session session = KruizeHibernateUtil.getSessionFactory().openSession()) {
```
To fully implement the timing behavior in line with `updateLayerToDB`, you will also need to:
1. Add `import io.micrometer.core.instrument.Timer;` and the appropriate `MetricsConfig` import at the top of this file if they are not already present (they likely are, since `updateLayerToDB` uses them).
2. In the success path (where the delete is committed and `validationOutputData` is marked successful), stop the timer with a success tag, e.g.:
`MetricsConfig.timerDeleteLayerDB().builder().tags("status", "success").register(MetricsConfig.meterRegistry).record(sample.stop(MetricsConfig.timerBDeleteLayerDB));`
or mirror exactly how `updateLayerToDB` constructs and stops its timers.
3. In each failure path (exceptions, `deletedCount == 0`, validation failure, etc.), stop the timer with a failure tag (e.g. `status=failed` or more granular tags), following the same pattern as `updateLayerToDB` for both which timer is used and which tags are set.
4. Ensure the timer is always stopped (e.g. in a `finally` block or via a helper method) so that no `Timer.Sample` is left unrecorded even on early returns or exceptions.
</issue_to_address>
### Comment 3
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_create_layer.py" line_range="698" />
<code_context>
+@pytest.mark.negative
+@pytest.mark.parametrize("test_name, expected_error_msg, layer_presence", [
+ ("label_empty_array", LAYER_PRESENCE_MISSING_MSG, '{"label": []}'),
+ ("label_null_element", "Validation failed: Label array contains null element", '{"label": [null, {"name": "test", "value": "test"}]}'),
+ ("label_duplicate_names", "Validation failed: Layer presence contains duplicate label names: runtime", '{"label": [{"name": "runtime", "value": "quarkus"}, {"name": "runtime", "value": "hotspot"}]}'),
+])
</code_context>
<issue_to_address>
**issue (testing):** The expected error message for null elements in the label array does not match the actual converter error message.
`Converters.convertInputJSONToCreateLayer` throws `IllegalArgumentException("Label array contains null elements")` (plural), and `LayerService.doPost` prefixes this with `"Validation failed: "`, so the runtime message is `"Validation failed: Label array contains null elements"`. This test currently expects `"Validation failed: Label array contains null element"` (singular), so it will fail despite correct behavior. Please update the expected string to match the implementation (ideally via a shared constant).
</issue_to_address>
### Comment 4
<location path="tests/scripts/helpers/utils.py" line_range="166" />
<code_context>
+TUNABLE_NULL_NAME_MSG = "Validation failed: Tunable name cannot be null"
+TUNABLE_EMPTY_NAME_MSG = "Validation failed: Tunable name cannot be empty"
+TUNABLE_MISSING_NAME_MSG = "Validation failed: Tunable object must have 'name' field"
+TUNABLE_NULL_IN_ARRAY_MSG = "Validation failed: Tunables array contains null element"
+TUNABLE_INVALID_VALUE_TYPE_MSG = "Validation failed: Tunable '%s' has invalid value_type. Must be one of: double, integer, categorical"
+TUNABLE_NULL_VALUE_TYPE_MSG = "Validation failed: Tunable '%s' has null value_type"
</code_context>
<issue_to_address>
**issue (bug_risk):** The tunables null-element error constant does not match the actual message produced by the converter and service.
`Converters.convertInputJSONToCreateLayer` throws `IllegalArgumentException("Tunables array contains null elements")`, which `LayerService.doPost` wraps as `"Validation failed: " + e.getMessage()`, producing `"Validation failed: Tunables array contains null elements"` on the wire. This constant instead uses the singular "element" and inlines the `"Validation failed: "` prefix, so it doesn’t match the actual API response and breaks tests like `"null_tunable_in_array"` in `test_create_layer_mandatory_fields_validation`. Please either update the constant to exactly match the runtime message (plural, no extra prefix), or update the tests to compose the expected message from the raw exception text so they track the real API behavior.
</issue_to_address>
### Comment 5
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_create_layer.py" line_range="239" />
<code_context>
+ ("null_tunable_in_array", TUNABLE_NULL_IN_ARRAY_MSG, "recommender.com/v1", "KruizeLayer", "test-meta", "test-layer", "test layer", '{"presence": "always"}', '[null, {"name": "t1", "value_type": "double", "upper_bound": "100", "lower_bound": "10", "step": 1}]'),
</code_context>
<issue_to_address>
**issue (bug_risk):** Because of the constant definition, this test’s expected message will not match the actual error returned by the API for null tunables.
The service returns `"Validation failed: Tunables array contains null elements"`, but `TUNABLE_NULL_IN_ARRAY_MSG` is `"Validation failed: Tunables array contains null element"`, so this test will assert the wrong message. After updating the constant or how the expectation is built, please align this test to the exact string returned by the API.
</issue_to_address>
### Comment 6
<location path="tests/scripts/local_monitoring_tests/rest_apis/test_list_layers.py" line_range="401" />
<code_context>
+
+@pytest.mark.layers
+@pytest.mark.sanity
+def test_list_layers_performance_with_many_layers(cluster_type):
+ """
+ Test Description: This test validates listLayers API performance when listing 100+ layers.
</code_context>
<issue_to_address>
**nitpick (testing):** The performance test’s documentation and implementation are slightly out of sync and could be made clearer or more flexible.
The docstring says this validates performance for 100+ layers, but the test only creates 10 (per the in-code comment). Please either update the docstring to reflect the current behavior or make `num_layers` configurable (e.g., a constant, env var, or marker-controlled value) so it can be scaled up when needed while staying fast by default.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| // Delete from database | ||
| ValidationOutputData deletedFromDB = experimentDAO.deleteLayerByName(layerName); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Distinguish between true server errors and not-found cases during delete to avoid returning 500 for a missing layer.
There’s a race between loadLayerByName and deleteLayerByName: if the layer is removed in between, deletedFromDB.isSuccess() will be false and we’ll return a 500 even though the message says "Layer not found" (should be 404). Consider making the DAO the single source of truth: either map its "not found" result to 404 here, or drop the pre-check and rely entirely on the delete call for existence and status mapping.
| @@ -689,6 +689,83 @@ public ValidationOutputData addLayerToDB(KruizeLMLayerEntry kruizeLayerEntry) { | |||
| return validationOutputData; | |||
Contributor
There was a problem hiding this comment.
suggestion: Consider adding metrics timing for deleteLayerByName similar to other DB operations.
MetricsConfig already defines timerBDeleteLayerDB / timerDeleteLayerDB, and updateLayerToDB uses a Timer.Sample. deleteLayerByName currently doesn’t record timing; consider wrapping it in a Timer.Sample with success/failure tags for consistent observability with other DB operations.
Suggested implementation:
@Override
public ValidationOutputData deleteLayerByName(String layerName) {
Timer.Sample sample = Timer.start(MetricsConfig.meterRegistry);
ValidationOutputData validationOutputData = new ValidationOutputData(false, null, null);
Transaction tx = null;
try (Session session = KruizeHibernateUtil.getSessionFactory().openSession()) {To fully implement the timing behavior in line with updateLayerToDB, you will also need to:
- Add
import io.micrometer.core.instrument.Timer;and the appropriateMetricsConfigimport at the top of this file if they are not already present (they likely are, sinceupdateLayerToDBuses them). - In the success path (where the delete is committed and
validationOutputDatais marked successful), stop the timer with a success tag, e.g.:
MetricsConfig.timerDeleteLayerDB().builder().tags("status", "success").register(MetricsConfig.meterRegistry).record(sample.stop(MetricsConfig.timerBDeleteLayerDB));
or mirror exactly howupdateLayerToDBconstructs and stops its timers. - In each failure path (exceptions,
deletedCount == 0, validation failure, etc.), stop the timer with a failure tag (e.g.status=failedor more granular tags), following the same pattern asupdateLayerToDBfor both which timer is used and which tags are set. - Ensure the timer is always stopped (e.g. in a
finallyblock or via a helper method) so that noTimer.Sampleis left unrecorded even on early returns or exceptions.
| @pytest.mark.negative | ||
| @pytest.mark.parametrize("test_name, expected_error_msg, layer_presence", [ | ||
| ("label_empty_array", LAYER_PRESENCE_MISSING_MSG, '{"label": []}'), | ||
| ("label_null_element", "Validation failed: Label array contains null element", '{"label": [null, {"name": "test", "value": "test"}]}'), |
Contributor
There was a problem hiding this comment.
issue (testing): The expected error message for null elements in the label array does not match the actual converter error message.
Converters.convertInputJSONToCreateLayer throws IllegalArgumentException("Label array contains null elements") (plural), and LayerService.doPost prefixes this with "Validation failed: ", so the runtime message is "Validation failed: Label array contains null elements". This test currently expects "Validation failed: Label array contains null element" (singular), so it will fail despite correct behavior. Please update the expected string to match the implementation (ideally via a shared constant).
| TUNABLE_NULL_NAME_MSG = "Validation failed: Tunable name cannot be null" | ||
| TUNABLE_EMPTY_NAME_MSG = "Validation failed: Tunable name cannot be empty" | ||
| TUNABLE_MISSING_NAME_MSG = "Validation failed: Tunable object must have 'name' field" | ||
| TUNABLE_NULL_IN_ARRAY_MSG = "Validation failed: Tunables array contains null element" |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The tunables null-element error constant does not match the actual message produced by the converter and service.
Converters.convertInputJSONToCreateLayer throws IllegalArgumentException("Tunables array contains null elements"), which LayerService.doPost wraps as "Validation failed: " + e.getMessage(), producing "Validation failed: Tunables array contains null elements" on the wire. This constant instead uses the singular "element" and inlines the "Validation failed: " prefix, so it doesn’t match the actual API response and breaks tests like "null_tunable_in_array" in test_create_layer_mandatory_fields_validation. Please either update the constant to exactly match the runtime message (plural, no extra prefix), or update the tests to compose the expected message from the raw exception text so they track the real API behavior.
| ("tunable_null_name", TUNABLE_NULL_NAME_MSG, "recommender.com/v1", "KruizeLayer", "test-meta", "test-layer", "test layer", '{"presence": "always"}', '[{"name": null, "value_type": "double", "upper_bound": "100", "lower_bound": "10", "step": 1}]'), | ||
| ("tunable_empty_name", TUNABLE_EMPTY_NAME_MSG, "recommender.com/v1", "KruizeLayer", "test-meta", "test-layer", "test layer", '{"presence": "always"}', '[{"name": "", "value_type": "double", "upper_bound": "100", "lower_bound": "10", "step": 1}]'), | ||
| ("tunable_missing_name", TUNABLE_MISSING_NAME_MSG, "recommender.com/v1", "KruizeLayer", "test-meta", "test-layer", "test layer", '{"presence": "always"}', '[{"value_type": "double", "upper_bound": "100", "lower_bound": "10", "step": 1}]'), | ||
| ("null_tunable_in_array", TUNABLE_NULL_IN_ARRAY_MSG, "recommender.com/v1", "KruizeLayer", "test-meta", "test-layer", "test layer", '{"presence": "always"}', '[null, {"name": "t1", "value_type": "double", "upper_bound": "100", "lower_bound": "10", "step": 1}]'), |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Because of the constant definition, this test’s expected message will not match the actual error returned by the API for null tunables.
The service returns "Validation failed: Tunables array contains null elements", but TUNABLE_NULL_IN_ARRAY_MSG is "Validation failed: Tunables array contains null element", so this test will assert the wrong message. After updating the constant or how the expectation is built, please align this test to the exact string returned by the API.
|
|
||
| @pytest.mark.layers | ||
| @pytest.mark.sanity | ||
| def test_list_layers_performance_with_many_layers(cluster_type): |
Contributor
There was a problem hiding this comment.
nitpick (testing): The performance test’s documentation and implementation are slightly out of sync and could be made clearer or more flexible.
The docstring says this validates performance for 100+ layers, but the test only creates 10 (per the in-code comment). Please either update the docstring to reflect the current behavior or make num_layers configurable (e.g., a constant, env var, or marker-controlled value) so it can be scaled up when needed while staying fast by default.
|
This pull request has been marked as stale due to 30 days of inactivity. Please comment if this pull request is still relevant to keep it active. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please describe the issue or feature and the summary of changes made to fix this.
Fixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Add full CRUD support and enhanced validation for layer management APIs, and expand test coverage around create/list layer behaviors including label-based detection, query/label/tunable validation, and performance characteristics.
New Features:
Enhancements:
Tests: