Repository navigation
fix(verifier): auto-validate managed_xprof xprof_url and honor expected_title_pattern - #1501
CatherineF-dev wants to merge 2 commits into
Conversation
…ed_title_pattern Nightly workload verification previously checked only whether a ManagedXprofArtifact record existed in Experiment State Server, without verifying that the Reconciler enriched payload.xprof.xprof_url on clusters configured with a Managed XProf GCS bucket, and ignored CLI overrides for --expected_title_pattern. - Add --require_managed_xprof_url=auto|true|false (default: auto), inspecting ConfigMap managed-xprof-gcs-config in namespace xmc when set to auto so clusters configured with Managed XProf require a non-empty enriched xprof_url while CI clusters without a bucket continue verifying artifact registration cleanly. - Validate ManagedXprofArtifact display_name, URL scheme, and top-level artifact.url consistency in xmc_entities_validator.validate_artifacts, and surface pending URL timeouts with actionable diagnostics. - Honor the --expected_title_pattern CLI flag override in verify_standard_workload, and guard verifier_cli.get_flag against UnparsedFlagAccessError when verification functions are invoked directly from unit tests. TAG=agy CONV=108bc446-88fa-4cbf-b70b-e3e5e9c917c5
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to enforce verification of xprof_url for ManagedXprof artifacts, controlled by a new require_managed_xprof_url flag. It adds logic to dynamically detect if the target Kubernetes cluster has a managed xprof bucket using kubectl and validates the artifact metadata accordingly. Unit tests have been added to cover these new verification flows. The review feedback suggests refactoring nested getattr calls in _extract_managed_xprof_details to improve readability, and making _normalize_artifact_type public in xmc_entities_validator.py instead of accessing it as a private member from workload_verification.py.
| xprof_url = str(getattr(getattr(payload, "xprof", None), "xprof_url", "") or "").strip() | ||
| top_url = str(getattr(getattr(art, "url", None), "url", "") or "").strip() |
There was a problem hiding this comment.
The nested getattr calls with fallback defaults are quite hard to read and maintain. In Python, it is much more idiomatic and readable to retrieve the intermediate objects first and perform a simple check, or use a helper function.
| xprof_url = str(getattr(getattr(payload, "xprof", None), "xprof_url", "") or "").strip() | |
| top_url = str(getattr(getattr(art, "url", None), "url", "") or "").strip() | |
| xprof_url = "" | |
| if (xprof_obj := getattr(payload, "xprof", None)) is not None: | |
| xprof_url = str(getattr(xprof_obj, "xprof_url", "") or "").strip() | |
| top_url = "" | |
| if (art_url_obj := getattr(art, "url", None)) is not None: | |
| top_url = str(getattr(art_url_obj, "url", "") or "").strip() |
| normalized = { | ||
| xmc_entities_validator._normalize_artifact_type(t) for t in expected_artifact_types | ||
| } |
There was a problem hiding this comment.
Accessing a protected/private member (prefixed with _) of another module is discouraged as it violates encapsulation and can lead to maintainability issues. Consider renaming _normalize_artifact_type to normalize_artifact_type in xmc_entities_validator.py to make it public, and reference the public function here.
| normalized = { | |
| xmc_entities_validator._normalize_artifact_type(t) for t in expected_artifact_types | |
| } | |
| normalized = { | |
| xmc_entities_validator.normalize_artifact_type(t) for t in expected_artifact_types | |
| } |
References
- PEP 8 recommends that public attributes and functions should have no leading underscores. Internal/non-public APIs should be prefixed with a leading underscore and should not be accessed from outside their defining module. (link)
| "auto", | ||
| ["auto", "true", "false"], | ||
| "Whether ManagedXprof artifacts must expose an enriched xprof_url " | ||
| "(auto enforces when ConfigMap managed-xprof-gcs-config exists in K8s).", |
There was a problem hiding this comment.
also, the xmc instance has been upgraded to enable Managed Xprof feature
…tr checks - Rename _normalize_artifact_type to public normalize_artifact_type in xmc_entities_validator.py instead of calling a protected helper across modules. - Simplify nested getattr calls in _extract_managed_xprof_details using explicit intermediate checks. TAG=agy
d9de688 to
58889d2
Compare
PR Stack
fix(reconciler): cache mlrun urls across batch flush window and downgrade 409 logsfix(verifier): auto-validate managed_xprof xprof_url and honor expected_title_pattern👉 Current PRfix(examples): eliminate managed_xprof startup race and align image nameSummary
Adds
--require_managed_xprof_url=auto|true|false(default:auto) to the nightly workload verification CLI so clusters configured with Managed XProf validate thatManagedXprofArtifactrecords receive an enrichedxprof_url, and honors--expected_title_patternCLI flag overrides.Motivation & Context
Previously, nightly verification checked only that a
ManagedXprofArtifactexisted in Experiment State Server without verifying that the Reconciler enrichedpayload.xprof.xprof_urlon clusters configured with a Managed XProf GCS bucket (ConfigMap managed-xprof-gcs-config), and ignored--expected_title_patternoverrides inverify_standard_workload.Key Changes
--require_managed_xprof_url(auto|true|false) intools/nightly_tests/verification/verifier_cli.pyandcluster_has_managed_xprof_bucket()/resolve_require_managed_xprof_url()intools/nightly_tests/verification/workload_verification.py.tools/nightly_tests/verification/xmc_entities_validator.pyto validateManagedXprofArtifactdisplay_name, URL scheme, and top-levelartifact.urlconsistency, and surface clear timeout diagnostics whenxprof_urlremains empty.--expected_title_patterninverify_standard_workloadand guardedverifier_cli.get_flagagainstUnparsedFlagAccessError.Assumptions
None — requirements were fully specified.
Verification & Testing
python3 -m unittest tools/nightly_tests/verification/verifier_cli_test.py tools/nightly_tests/verification/workload_verification_test.py tools/nightly_tests/verification/xmc_entities_validator_test.pyRelated Issues / Bugs
N/A
TAG=agy
CONV=108bc446-88fa-4cbf-b70b-e3e5e9c917c5