feat(dataqualityrule): added data quality rule support - #66
feat(dataqualityrule): added data quality rule support#66jacopocinaark wants to merge 29 commits into
Conversation
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…Klab/Artesian.SDK-Python into feature/22668-DataQualityRule
There was a problem hiding this comment.
Pull request overview
Adds Data Quality Rule (DQR) support to the Python SDK’s MarketData surface, including DTOs/enums, MarketDataService endpoints, tests, and usage docs/samples.
Changes:
- Added
MarketDataServiceCRUD APIs for data quality rules and rule assignments, plus an assignment events feed endpoint. - Introduced Data Quality DTOs/enums (rule types, schedules, outlier models, paged results, assignments, events, status summary).
- Extended tests, README documentation, and added runnable samples for rule/assignment flows.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/TestMarketDataService.py | Adds unit tests for DQR CRUD and assignment CRUD. |
| src/Artesian/MarketData/MarketDataService.py | Adds DQR and assignment endpoints to the service, including an events feed method. |
| src/Artesian/MarketData/_Enum/ScheduleDefinitionType.py | Adds schedule definition discriminator enum. |
| src/Artesian/MarketData/_Enum/RuleType.py | Adds DQ rule type enum (CompletenessAndFreshness/Outlier). |
| src/Artesian/MarketData/_Enum/PeriodPrecision.py | Adds precision enum used by period-based configs. |
| src/Artesian/MarketData/_Enum/OutlierModel.py | Adds outlier model discriminator enum. |
| src/Artesian/MarketData/_Enum/MarketDataTypeV2.py | Adds “v2” market data type enum for DQ configs. |
| src/Artesian/MarketData/_Enum/CheckAggregatedStatus.py | Adds aggregated status enum (OK/KO). |
| src/Artesian/MarketData/_Enum/init.py | Updates enum package exports (currently incomplete for new public enums). |
| src/Artesian/MarketData/_Dto/VersionedCompletenessAndFreshnessConfigDto.py | Adds versioned completeness/freshness config DTO. |
| src/Artesian/MarketData/_Dto/ScheduleDefinitionDto.py | Adds base schedule definition DTO abstraction. |
| src/Artesian/MarketData/_Dto/ScheduleConfigDto.py | Adds schedule config DTO (definition + maxDelay). |
| src/Artesian/MarketData/_Dto/RecordValidationConfigDto.py | Adds record validation window DTO. |
| src/Artesian/MarketData/_Dto/PagedResult.py | Adds paged result wrappers for DQRs and assignments. |
| src/Artesian/MarketData/_Dto/OutlierRefCurveConfigDto.py | Adds reference-curve outlier model config DTO. |
| src/Artesian/MarketData/_Dto/OutlierModelConfigDto.py | Adds base outlier model config DTO (currently has constructor issue). |
| src/Artesian/MarketData/_Dto/OutlierConfigDto.py | Adds outlier rule configuration DTO. |
| src/Artesian/MarketData/_Dto/OutlierAbsoluteBoundConfigDto.py | Adds absolute-bounds outlier model config DTO. |
| src/Artesian/MarketData/_Dto/MarketDataQualityRuleAssignmentDto.py | Adds rule assignment DTOs (input/output). |
| src/Artesian/MarketData/_Dto/DqCheckChangeEventDto.py | Adds DQ change-event DTOs for assignment event feed. |
| src/Artesian/MarketData/_Dto/DataQualityStatusSummaryDto.py | Adds status summary DTO (currently has keyword/serialization mapping risk). |
| src/Artesian/MarketData/_Dto/DataQualityRuleDtoOutput.py | Adds rule output DTO (adds aggregatedStatus). |
| src/Artesian/MarketData/_Dto/DataQualityRuleDtoInput.py | Adds rule input DTO. |
| src/Artesian/MarketData/_Dto/DataQualityRuleConfigDto.py | Adds base config DTO with type discriminator. |
| src/Artesian/MarketData/_Dto/CronScheduleDefinitionDto.py | Adds cron-based schedule definition DTO. |
| src/Artesian/MarketData/_Dto/CompletenessAndFreshnessConfigDto.py | Adds completeness/freshness base config DTO. |
| src/Artesian/MarketData/_Dto/ActualCompletenessAndFreshnessConfigDto.py | Adds “actual time series” completeness/freshness config DTO. |
| src/Artesian/MarketData/_Dto/init.py | Exposes new DTOs via the DTO package exports. |
| samples/TestDataQualityAssignment.py | Adds a manual end-to-end sample for rule assignment lifecycle. |
| samples/TestDataQuality.py | Adds a manual sample for rule CRUD lifecycle. |
| README.md | Documents Data Quality Rules usage and updates formatting in other sections. |
Comments suppressed due to low confidence (2)
src/Artesian/MarketData/MarketDataService.py:802
marketDataIdandruleIdare always added to query params even when None. This risks sendingmarketDataId=None/ruleId=None; omit them when not provided.
params["page"] = page
params["pageSize"] = pageSize
params["marketDataId"] = marketDataId
params["ruleId"] = ruleId
if ruleName:
src/Artesian/MarketData/MarketDataService.py:795
- Pagination validation error messages contain grammatical errors ("must to be") and report constraints inconsistently (code enforces 1-based pages). Consider clearer, structured messages.
if pageSize < 1:
raise ValueError(
"PageSize must to be greater than 0. Page Size:" + str(pageSize)
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (4)
src/Artesian/MarketData/MarketDataService.py:527
marketDataIdis always added to query params, even when it is None. Withrequests, this can result inmarketDataId=Nonebeing sent, which changes the meaning of the request. Only include this filter when a value is provided.
if type is not None:
params["type"] = type.name
params["marketDataId"] = marketDataId
if name:
src/Artesian/MarketData/MarketDataService.py:793
- The validation error messages here are ungrammatical/inconsistent ("must to be") and differ from the style used elsewhere in this file. Prefer the same
page must be >= 1 (got X)format used inreadDataQualityRuleAsync.
if page < 1:
raise ValueError("Page must to be greater than 0. Page:" + str(page))
if pageSize < 1:
raise ValueError(
"PageSize must to be greater than 0. Page Size:" + str(pageSize)
)
src/Artesian/MarketData/MarketDataService.py:953
- New
readDataQualityRuleAssignmentEventsFeedAsyncbehavior is not covered by unit tests (endpoint path andafterTimestampquery serialization). This file already has extensive request-matching tests, so this looks like an accidental gap.
async def readDataQualityRuleAssignmentEventsFeedAsync(
self: MarketDataService,
id: int,
afterTimestamp: Optional[datetime] = None,
) -> List[DqCheckChangeEventDtoOutput]:
src/Artesian/MarketData/_Dto/DataQualityStatusSummaryDto.py:35
- Field name
from_will serialize to JSON keyFrom_with the current global key transformer (__camelToPascalonly uppercases the first letter). The docstring says the API field name isFrom, so this DTO likely won't round-trip correctly unless the serializer strips the trailing underscore or a per-field rename is configured.
from_: Optional[date] = None
to: Optional[date] = None
…Klab/Artesian.SDK-Python into feature/22668-DataQualityRule
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (4)
src/Artesian/MarketData/MarketDataService.py:496
typeis documented as an optional filter, but it's a required positional argument in the signature. This forces callers to always pass a value (or explicitly passNone), which is inconsistent with the docstring and other optional query filters.
self: MarketDataService,
page: int,
pageSize: int,
type: Optional[RuleType],
marketDataId: Optional[int] = None,
src/Artesian/MarketData/MarketDataService.py:800
marketDataIdandruleIdare optional filters, but they are always added to the query params even whenNone. Withrequests, this can end up sendingmarketDataId=None/ruleId=Noneon the wire, which changes server-side filtering semantics.
params = {}
params["page"] = page
params["pageSize"] = pageSize
params["marketDataId"] = marketDataId
params["ruleId"] = ruleId
src/Artesian/MarketData/MarketDataService.py:794
- The validation error messages have grammatical issues ("must to be") and are inconsistent with the clearer f-string format used elsewhere in this file (e.g.,
readDataQualityRuleAsync). This is a public-facing exception message.
raise ValueError("Page must to be greater than 0. Page:" + str(page))
if pageSize < 1:
raise ValueError(
"PageSize must to be greater than 0. Page Size:" + str(pageSize)
)
src/Artesian/MarketData/MarketDataService.py:954
- This new API surface (
readDataQualityRuleAssignmentEventsFeed*) has no unit test coverage intests/TestMarketDataService.py, unlike the other newly added Data Quality Rule endpoints. Add aresponses-based test to lock down the query param serialization (especiallyafterTimestamp) and the list deserialization behavior.
async def readDataQualityRuleAssignmentEventsFeedAsync(
self: MarketDataService,
id: int,
afterTimestamp: Optional[datetime] = None,
) -> List[DqCheckChangeEventDtoOutput]:
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (8)
src/Artesian/MarketData/MarketDataService.py:796
- Error messages for page/pageSize validation are inconsistent with other pagination methods in this file and contain grammatical errors ("must to be"). Prefer the same >= 1 (got X) format used elsewhere for clearer API errors.
if page < 1:
raise ValueError("Page must to be greater than 0. Page:" + str(page))
if pageSize < 1:
raise ValueError(
"PageSize must to be greater than 0. Page Size:" + str(pageSize)
)
src/Artesian/MarketData/MarketDataService.py:802
- readDataQualityRuleAssignmentAsync currently always includes marketDataId/ruleId in query params even when they are None. That can send
marketDataId=None/ruleId=Noneto the API and change server-side filtering behavior. Only include these params when a value is provided (same pattern as readDataQualityRuleAsync).
params = {}
params["page"] = page
params["pageSize"] = pageSize
params["marketDataId"] = marketDataId
params["ruleId"] = ruleId
src/Artesian/MarketData/MarketDataService.py:984
- New API surface readDataQualityRuleAssignmentEventsFeedAsync/readDataQualityRuleAssignmentEventsFeed is not covered by tests, while this module has extensive response-mocking coverage for other MarketDataService endpoints.
async def readDataQualityRuleAssignmentEventsFeedAsync(
self: MarketDataService,
id: int,
afterTimestamp: Optional[datetime] = None,
) -> List[DqCheckChangeEventDtoOutput]:
"""
Retrieves the raw event feed for a specific rule assignment.
Args:
id: rule assignment identifier.
afterTimestamp: optional lower bound, returns events after instant.
Returns:
List of DqCheckChangeEventDtoOutput (Async).
"""
url = "/dataquality/dqruleassignment/" + str(id) + "/events"
params = {}
if afterTimestamp is not None:
params["afterTimestamp"] = afterTimestamp.isoformat()
with self.__client as c:
res = await asyncio.gather(
*[
self.__executor.exec(
c.exec,
"GET",
url,
None,
retcls=List[DqCheckChangeEventDtoOutput],
params=params,
)
]
)
return cast(List[DqCheckChangeEventDtoOutput], res[0])
src/Artesian/MarketData/_Dto/ScheduleDefinitionDto.py:16
- ScheduleDefinitionDto defines
typeas a @Property. jsons/dataclass serialization typically only serializes dataclass fields, so the discriminator may be omitted from JSON. Maketypea dataclass field (init=False) and let subclasses provide the default so the discriminator is reliably serialized.
@dataclass
class ScheduleDefinitionDto:
"""
Base class for schedule definition DTOs.
"""
@property
def type(self: "ScheduleDefinitionDto") -> ScheduleDefinitionType:
raise NotImplementedError(
"ScheduleDefinitionDto.type must be implemented by subclasses"
)
src/Artesian/MarketData/_Dto/CronScheduleDefinitionDto.py:24
- CronScheduleDefinitionDto exposes the schedule discriminator via a @Property. If the API expects a
typefield in the payload, this may not be serialized. Prefer a dataclass field (init=False) with a default so it is always present in JSON.
@dataclass
class CronScheduleDefinitionDto(ScheduleDefinitionDto):
"""
A schedule definition based on a cron expression, specifying recurring
check times in a given time zone.
Attributes:
cronExpression: cron expression defining the schedule pattern
timeZone: IANA time zone identifier used to evaluate cronExpression
"""
cronExpression: Optional[str] = None
timeZone: Optional[str] = None
@property
def type(self: "CronScheduleDefinitionDto") -> ScheduleDefinitionType:
return ScheduleDefinitionType.Cron
src/Artesian/MarketData/_Dto/OutlierModelConfigDto.py:17
- OutlierModelConfigDto defines
modelas a @Property. Ifmodelis a required discriminator for outlier configs, it may be omitted from serialized JSON. Prefer a dataclass field (init=False) and let subclasses set the default discriminator value.
@dataclass
class OutlierModelConfigDto(DataQualityRuleConfigDto):
"""
Base configuration for outlier detection rules.
"""
@property
def model(self: "OutlierModelConfigDto") -> OutlierModel:
raise NotImplementedError(
"OutlierModelConfigDto.model must be implemented by subclasses"
)
src/Artesian/MarketData/_Dto/OutlierAbsoluteBoundConfigDto.py:25
- OutlierAbsoluteBoundConfigDto exposes
modelvia a @Property. Ifmodelmust be part of the JSON payload for polymorphic deserialization server-side, this likely won’t be serialized. Use a dataclass field (init=False) with a default discriminator value instead.
@dataclass
class OutlierAbsoluteBoundConfigDto(OutlierModelConfigDto):
"""
Outlier detection model using fixed absolute bounds.
A data point is flagged as an outlier if its value falls below
lowerBound or above upperBound.
Attributes:
upperBound: maximum acceptable value
lowerBound: minimum acceptable value
"""
upperBound: float
lowerBound: float
@property
def model(self: "OutlierAbsoluteBoundConfigDto") -> OutlierModel:
return OutlierModel.AbsoluteBound
src/Artesian/MarketData/_Dto/OutlierRefCurveConfigDto.py:25
- OutlierRefCurveConfigDto exposes
modelvia a @Property. Ifmodelmust be present in JSON to discriminate between outlier model subtypes, it may be omitted from serialization. Use a dataclass field (init=False) with a default discriminator value instead.
@dataclass
class OutlierRefCurveConfigDto(OutlierModelConfigDto):
"""
Outlier detection model based on a reference Market Data curve.
A data point is flagged as an outlier if it deviates from the
reference value by more than tolerancePerc.
Attributes:
referenceMarketDataId: id of the reference Market Data entity
tolerancePerc: maximum allowed percentage deviation from reference
"""
referenceMarketDataId: int
tolerancePerc: float
@property
def model(self: "OutlierRefCurveConfigDto") -> OutlierModel:
return OutlierModel.RefCurve
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 46 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
src/Artesian/MarketData/MarketDataService.py:802
readDataQualityRuleAssignmentAsyncalways includesmarketDataIdandruleIdin query params even when they areNone. Unlike other methods in this file, this can emit unwanted query parameters (e.g.ruleId=None) and change server-side filtering behavior.
params["marketDataId"] = marketDataId
params["ruleId"] = ruleId
src/Artesian/MarketData/_Dto/OutlierModelConfigDto.py:12
OutlierModelConfigDtoinheritsDataQualityRuleConfigDto, so its subclasses (e.g.OutlierAbsoluteBoundConfigDto) require callers to passtype=...even though this is alwaysRuleType.Outlier. That’s error-prone (callers can pass the wrong discriminator) and inconsistent with other config DTOs that fixtypeviafield(init=False, default=...).
class OutlierModelConfigDto(DataQualityRuleConfigDto):
"""
Base configuration for outlier detection rules.
"""
src/Artesian/MarketData/_Enum/MarketDataTypeV2.py:4
- This PR is titled/linked as adding Data Quality Rule support, but it also renames/removes the public
MarketDataTypeenum (nowMarketDataTypeV2) and updates exports. That is a potentially breaking API change unrelated to data quality rules; consider either restoring backwards compatibility (alias/stub module + re-export) or calling out the breaking change explicitly in the PR description/release notes.
src/Artesian/MarketData/_Enum/init.py:17 __all__containsMarketDataTypeV2.__name__twice, which can lead to duplicate exports and is likely unintended.
src/Artesian/MarketData/MarketDataService.py:796- Validation error messages for
page/pageSizeinreadDataQualityRuleAssignmentAsynchave grammar issues ("must to be") and are inconsistent with the clearer f-string style used elsewhere in this file (e.g.readDataQualityRuleAsync).
raise ValueError("Page must to be greater than 0. Page:" + str(page))
if pageSize < 1:
raise ValueError(
"PageSize must to be greater than 0. Page Size:" + str(pageSize)
)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated no new comments.
Suppressed comments (8)
src/Artesian/MarketData/_Dto/init.py:120
DataQualityStatusSummaryDtois listed twice in__all__(already present earlier in the list). This duplication is unnecessary.
src/Artesian/MarketData/_Enum/init.py:19__all__containsMarketDataTypeV2.__name__twice, which is redundant and can confuse tooling that inspects exports.
src/Artesian/MarketData/_Dto/init.py:14- The
from .PagedResult import (...)block uses leading commas on continued lines, which is hard to read and likely to trip format/lint rules. Reformat it consistently with the rest of the file.
This issue also appears on line 118 of the same file.
src/Artesian/MarketData/MarketDataService.py:1173
- These validation error messages have grammatical issues ("must to be") and are inconsistent with other pagination validations in this class (which use
page must be >= 1 (got X)).
if page < 1:
raise ValueError("Page must to be greater than 0. Page:" + str(page))
if pageSize < 1:
raise ValueError(
"PageSize must to be greater than 0. Page Size:" + str(pageSize)
)
src/Artesian/MarketData/_Dto/MarketDataDqStatusSummaryDto.py:25
assignmentsuses a bareListtype, which loses element typing information and is inconsistent with the other DTOs in this module that specify list element types.
README.md:508- This README code sample is malformed:
versionToleranceFrom/ToandversionPrecisionare fields ofVersionedCompletenessAndFreshnessConfigDto, but they are currently placed inside theRecordValidationConfigDto(...)call, and the parentheses/indentation don’t match valid Python.
recordValidationConfig=RecordValidationConfigDto(
recordRangeFrom="P0D",
versionToleranceFrom="-PT1H",
versionToleranceTo="PT1H",
versionPrecision=PeriodPrecision.Hour,
README.md:532
- This README sample passes
type=RuleType.OutlierintoOutlierAbsoluteBoundConfigDto(...), butOutlierModelConfigDtodefinestypewithinit=False, so this will raiseTypeError: __init__() got an unexpected keyword argument 'type'.
model=OutlierAbsoluteBoundConfigDto(
lowerBound=-10.0,
upperBound=45.0,
type=RuleType.Outlier
)
src/Artesian/Query/QueryService.py:22
QueryServicenow targets the beta API (v1.1-beta) by default. That’s a behavioral change for all query users and can break consumers if the beta endpoint isn’t available/compatible; consider keeping the stable default and making beta opt-in.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
Previously missed (5) — in code that hasn't changed since the last review.
README.md:519
- README example for
VersionedCompletenessAndFreshnessConfigDtois malformed:versionTolerance*andversionPrecisionare not indented under the configuration object, and theRecordValidationConfigDtocall is never closed properly. As written, this code block won’t run and also putsversionTolerance*insideRecordValidationConfigDto(wrong type).
recordValidationConfig=RecordValidationConfigDto(
recordRangeFrom="P0D",
versionToleranceFrom="-PT1H",
versionToleranceTo="PT1H",
versionPrecision=PeriodPrecision.Hour,
README.md:543
- README outlier rule example passes
type=RuleType.OutliertoOutlierAbsoluteBoundConfigDto, butOutlierModelConfigDtodefinestypewithinit=False, so passing it will raiseTypeError: __init__() got an unexpected keyword argument 'type'.
model=OutlierAbsoluteBoundConfigDto(
lowerBound=-10.0,
upperBound=45.0,
type=RuleType.Outlier
)
src/Artesian/MarketData/_Enum/init.py:20
__all__containsMarketDataTypeV2.__name__twice, which is redundant and suggests a copy/paste mistake in the public export list.
src/Artesian/MarketData/_Dto/init.py:14- Import list formatting has leading commas inside the parenthesized import (
\n , PagedResultQuality...), which is easy to miss in reviews and can trip style/lint checks. Keep commas at line ends for readability.
tests/TestMarketDataService.py:94 setUp()mutatesQualityNotificationAlertDto*.__init__.__annotations__and never restores it. This leaks global state across tests (and will be flaky if tests are ever run in a different order / in parallel). UseaddCleanup()to restore the original annotation after each test.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 69 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
README.md:519
- The Versioned Completeness/Freshness example is syntactically invalid:
versionToleranceFrom/ToandversionPrecisionare indented as if they wereRecordValidationConfigDtofields, and theRecordValidationConfigDto(...)call is not closed. This code block won’t run as written.
recordValidationConfig=RecordValidationConfigDto(
recordRangeFrom="P0D",
versionToleranceFrom="-PT1H",
versionToleranceTo="PT1H",
versionPrecision=PeriodPrecision.Hour,
src/Artesian/MarketData/_Enum/init.py:20
__all__containsMarketDataTypeV2twice, which is redundant and can confusefrom ... import *exports.
src/Artesian/Query/QueryService.py:22
QueryServicenow hardcodes the beta API version (v1.1-beta) for all queries. This is an operational/API stability risk because existing users will silently switch to a beta backend without opt-in. If beta is needed, it should be selectable; otherwise keep the stable default.
src/Artesian/MarketData/MarketDataService.py:433- This method adds
includeCurveSummary/includeDataQualityquery params, but the response is still deserialized intoMarketDataEntityOutput, so any enriched fields returned by the API will be discarded. Deserialize intoMarketDataEntityOutputEnrichedto preserve them when requested.
url = "/marketdata/entity"
params = {
"provider": provider,
"curveName": curveName,
"includeCurveSummary": includeCurveSummary,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 69 changed files in this pull request and generated no new comments.
Suppressed comments (8)
Previously missed (5) — in code that hasn't changed since the last review.
src/Artesian/MarketData/_Enum/init.py:19
- MarketDataTypeV2 is listed twice in all. This duplicates the export list and is likely unintended.
src/Artesian/MarketData/_Dto/init.py:14 - The PagedResult re-export import block has leading commas and inconsistent indentation. This is hard to read and will likely be reformatted (or rejected) by isort/black-style tooling.
README.md:517 - The "Versioned Time Series" example is currently syntactically invalid (mis-indented args and missing RecordValidationConfigDto fields/closing). As written, users can’t copy/paste it successfully.
recordValidationConfig=RecordValidationConfigDto(
recordRangeFrom="P0D",
versionToleranceFrom="-PT1H",
versionToleranceTo="PT1H",
versionPrecision=PeriodPrecision.Hour,
README.md:542
- The outlier example passes type=RuleType.Outlier into OutlierAbsoluteBoundConfigDto, but OutlierModelConfigDto defines type with init=False. This will raise TypeError: init() got an unexpected keyword argument 'type'.
model=OutlierAbsoluteBoundConfigDto(
lowerBound=-10.0,
upperBound=45.0,
type=RuleType.Outlier
)
src/Artesian/MarketData/_Dto/CheckResultCheckSummaryDto.py:5
- To keep DTO types consistent with the rest of the Data Quality surface, CheckResultCheckSummaryDto should use CheckAggregatedStatus for aggregatedStatus. That requires importing the enum here.
This issue also appears on line 29 of the same file.
from dataclasses import dataclass
from typing import Optional
import datetime
from Artesian.MarketData._Dto.MarketDataQualityRuleAssignmentDto import MarketDataQualityRuleAssignmentDtoOutput
src/Artesian/MarketData/MarketDataService.py:309
- readMarketDataRegistryByIdAsync is annotated to return MarketDataEntityOutputEnriched, but the request executor deserializes using retcls=MarketDataEntityOutput. That will drop enriched fields like curveSummary/dataQualityStatusSummary when include* flags are enabled.
"GET",
url,
None,
retcls=MarketDataEntityOutput,
params=params,
src/Artesian/Query/QueryService.py:24
- QueryService now hard-codes a beta API version (v1.1-beta). Even if intentional, consumers may need to pin a stable version (or a future GA version) without patching the SDK. Consider making the query API version an optional constructor parameter.
src/Artesian/MarketData/_Dto/CheckResultCheckSummaryDto.py:33 - aggregatedStatus is currently typed as str; the rest of the SDK models aggregated DQ status as CheckAggregatedStatus. Typing this field as the enum makes serialization/deserialization and consumer code consistent.
lastCheckTime: datetime.datetime
rangeStart: datetime.date
rangeEnd: datetime.date
aggregatedStatus: str
assignment: Optional[MarketDataQualityRuleAssignmentDtoOutput] = None
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 69 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/Artesian/MarketData/_Enum/init.py:20
- all contains MarketDataTypeV2 twice, which is redundant and can cause confusing star-import/export behavior in docs/tools.
README.md:519 - The README example for VersionedCompletenessAndFreshnessConfigDto is malformed: versionToleranceFrom/versionToleranceTo/versionPrecision are indented as if they were RecordValidationConfigDto fields and the parentheses don’t close correctly, so the snippet isn’t runnable/copy-pastable.
recordValidationConfig=RecordValidationConfigDto(
recordRangeFrom="P0D",
versionToleranceFrom="-PT1H",
versionToleranceTo="PT1H",
versionPrecision=PeriodPrecision.Hour,
src/Artesian/MarketData/MarketDataService.py:1262
- These ValueError messages contain grammatical errors ("must to be") and are inconsistent with the clearer messages used by other new paging methods in this file.
if page < 1:
raise ValueError("Page must to be greater than 0. Page:" + str(page))
if pageSize < 1:
raise ValueError(
"PageSize must to be greater than 0. Page Size:" + str(pageSize)
)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 69 changed files in this pull request and generated no new comments.
Suppressed comments (6)
Previously missed (5) — in code that hasn't changed since the last review.
src/Artesian/MarketData/_Enum/init.py:20
__all__exportsMarketDataTypeV2twice, which is redundant and can confuse introspection / star-import users.
src/Artesian/MarketData/_Dto/CheckResultCheckSummaryDto.py:33CheckResultCheckSummaryDto.aggregatedStatusis typed asstr, while the rest of the new DQ API surface usesCheckAggregatedStatus(Enum). This inconsistency prevents enum-aware serialization/deserialization and makes the DTO harder to use correctly.
lastCheckTime: datetime.datetime
rangeStart: datetime.date
rangeEnd: datetime.date
aggregatedStatus: str
assignment: Optional[MarketDataQualityRuleAssignmentDtoOutput] = None
README.md:519
- The Versioned Completeness & Freshness example code is syntactically invalid (broken indentation / missing parentheses) and is missing required fields for
RecordValidationConfigDto(e.g.,recordRangeTo). As written, users cannot copy/paste it successfully.
versionedCompletenessRule = DataQualityRuleDtoInput(
name="Hourly forecast version check",
type=RuleType.CompletenessAndFreshness,
configuration=VersionedCompletenessAndFreshnessConfigDto(
marketDataType=MarketDataTypeV2.VersionedTimeSerie,
scheduleConfig=ScheduleConfigDto(
scheduleDefinition=CronScheduleDefinitionDto(
cronExpression="15 * * * *",
timeZone="UTC",
),
maxDelay="PT30M",
),
recordValidationConfig=RecordValidationConfigDto(
recordRangeFrom="P0D",
versionToleranceFrom="-PT1H",
versionToleranceTo="PT1H",
versionPrecision=PeriodPrecision.Hour,
),
README.md:545
- The Outlier rule example passes
type=RuleType.OutlierintoOutlierAbsoluteBoundConfigDto(...), buttypeis defined asinit=FalseinOutlierModelConfigDtoso this will raiseTypeError: __init__() got an unexpected keyword argument 'type'.
outlierRule = DataQualityRuleDtoInput(
name="Temperature outlier detection",
type=RuleType.Outlier,
configuration=OutlierConfigDto(
model=OutlierAbsoluteBoundConfigDto(
lowerBound=-10.0,
upperBound=45.0,
type=RuleType.Outlier
)
),
version=0,
)
tests/TestMarketDataService.py:99
setUp()mutatesQualityNotificationAlertDtoInput/Output.__init__.__annotations__globally to force a concretetriggerConfigtype. This can leak across tests (order-dependent) and is risky if tests run concurrently. Prefer restoring the original annotations viaaddCleanup().
src/Artesian/Query/QueryService.py:22
- QueryService now hard-codes the query API version to
v1.1-beta, which forces all query traffic onto a beta endpoint and can be a breaking / stability risk for existing SDK consumers. Consider keeping the default on the stable version and exposing the beta version via an explicit opt-in (e.g., constructor arg or separate service).
…Klab/Artesian.SDK-Python into feature/22668-DataQualityRule
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 69 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
Previously missed (5) — in code that hasn't changed since the last review.
src/Artesian/MarketData/_Enum/init.py:20
__all__includesMarketDataTypeV2.__name__twice, which is redundant and can confuse export lists / tooling. Remove the duplicate entry.
src/Artesian/MarketData/MarketDataService.py:1262- Error messages have grammatical issues ("must to be") and are inconsistent with other paging checks in this file (which use
page must be >= 1 (got …)). This is user-facing and makes debugging harder.
if page < 1:
raise ValueError("Page must to be greater than 0. Page:" + str(page))
if pageSize < 1:
raise ValueError(
"PageSize must to be greater than 0. Page Size:" + str(pageSize)
)
src/Artesian/MarketData/_Dto/CheckResultCheckSummaryDto.py:33
aggregatedStatusis modeled asstr, but the SDK introducesCheckAggregatedStatusand uses it for other DQ status fields. Keeping this as a string makes the DTO inconsistent and forces consumers to do string comparisons. Prefer the enum here too.
lastCheckTime: datetime.datetime
rangeStart: datetime.date
rangeEnd: datetime.date
aggregatedStatus: str
assignment: Optional[MarketDataQualityRuleAssignmentDtoOutput] = None
README.md:519
- The Versioned Time Series example is currently syntactically invalid (mis-indented arguments and missing parentheses / required
recordRangeTo). This makes the documentation copy/paste unusable.
versionedCompletenessRule = DataQualityRuleDtoInput(
name="Hourly forecast version check",
type=RuleType.CompletenessAndFreshness,
configuration=VersionedCompletenessAndFreshnessConfigDto(
marketDataType=MarketDataTypeV2.VersionedTimeSerie,
scheduleConfig=ScheduleConfigDto(
scheduleDefinition=CronScheduleDefinitionDto(
cronExpression="15 * * * *",
timeZone="UTC",
),
maxDelay="PT30M",
),
recordValidationConfig=RecordValidationConfigDto(
recordRangeFrom="P0D",
versionToleranceFrom="-PT1H",
versionToleranceTo="PT1H",
versionPrecision=PeriodPrecision.Hour,
),
version=0,
)
README.md:545
- The Outlier rule example passes
type=RuleType.OutlierintoOutlierAbsoluteBoundConfigDto(...), but that dataclass only definesupperBoundandlowerBound(itstypediscriminator isinit=False). The snippet will raiseTypeErrorif copied.
outlierRule = DataQualityRuleDtoInput(
name="Temperature outlier detection",
type=RuleType.Outlier,
configuration=OutlierConfigDto(
model=OutlierAbsoluteBoundConfigDto(
lowerBound=-10.0,
upperBound=45.0,
type=RuleType.Outlier
)
),
There was a problem hiding this comment.
🔵 Needs a closer look
It includes a couple of confirmed export/typing defects (duplicate __all__ entries; untyped DTO field) and a potentially breaking default switch to a beta query API version that should be resolved before approval.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
src/Artesian/MarketData/_Dto/MarketDataDqStatusSummaryDto.py:25
assignmentsis typed asOptional[List], which loses the element type and makes this DTO harder to use correctly (no type-checking / IDE help). If this is meant to hold rule assignments, type it explicitly.
src/Artesian/MarketData/_Dto/init.py:126__all__exportsDataQualityStatusSummaryDtotwice, which is redundant and can create noisy re-exports when using star imports.
src/Artesian/MarketData/_Enum/init.py:20__all__containsMarketDataTypeV2.__name__twice, which is redundant and can cause noisy re-exports when usingfrom Artesian.MarketData._Enum import *.
src/Artesian/Query/QueryService.py:22
- Switching the default query API version from
v1.0tov1.1-betais a breaking behavioral change for allQueryServiceconsumers and also opts everyone into a beta endpoint by default. Consider keeping the stable default and making the beta version opt-in (e.g., via an optional constructor parameter).
- Files reviewed: 78/79 changed files
- Comments generated: 0 new
- Review effort level: Lite
ref: #22668