Repository navigation
FEAT: List the saved registry entries that couldn't be restored (part 5) - #3965
Open
varunj-msft wants to merge 172 commits into
Open
varunj-msft wants to merge 172 commits into
varunj-msft wants to merge 172 commits into
Conversation
…API input External construction now rejects a parameter whose type includes Path unless the calling service passes the name in allowed_path_params after making the value safe. The converter service allows its upload parameters once they are stored, and the target service allows the server-set upload folder and the GitHubCopilotTarget working directory. The scorer-only check is removed, and the "Valid parameters" list leaves out path parameters callers cannot set.
The target catalog now applies the path rule to required parameters too, so a target that needs a disallowed path is left out instead of listed without the field, matching the scorer catalog. allowed_path_params is typed frozenset[str] so a bare string cannot turn the allow-list into a substring match.
The target catalog still lists a target whose required path parameter is its server-set upload folder when the server has one, and leaves it out otherwise.
# Conflicts: # tests/unit/backend/test_converter_service.py
…benchmark's recipes folder
The runtime status keeps its short summary. The restore log now also lists each field's full location and message, escaped, without the stored values, and with URL credentials removed. Credentials are removed before a location is shortened, so the summary can't keep part of one either.
Its text can carry a SAS URL. The 503 handler no longer logs it as a debug traceback, and the scenario suite service no longer attaches it to its warning; both log the storage message, which has no credentials.
Its constructor imports torch and checks for CUDA, which can hold the loop for seconds the first time. The target create now does both in a worker thread first, so the constructor, its errors, and the model-loading task it starts stay on the loop and are quick.
The 503 handler logs it at debug level and the scenario suite service in its warning, as before, but through redact_urls now, so a connection failure can be told apart from other outages without the SAS query reaching the log.
Richard Lundeen (richlundeen)
left a comment
Richard Lundeen (richlundeen)
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the current head, including rendered UI states. Three new P2 findings are below. Three existing P2 concerns also remain open, so this review does not approve the PR.
Configuration lists the failures but can't fix them here, so the banner says to view them there and fix or delete the files in registry_recipes_source. It uses the multiline layout, since its longer text replaces the status after the bar first lays out.
…on a narrow screen
This branch has not been deployed
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
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
When a saved entry can't be restored, the backend stays not ready (#3909), but only administrators can see why, through
GET /api/config/runtime, and everyone else only sees that PyRIT isn't ready. This shows administrators the failures on the Configuration page and tells everyone what has to happen. Until the earlier PRs merge, the diff here also shows their changes.GET /api/config/runtime, which only administrators can read: each entry's kind and name, the full message, and the entry it needed when the message doesn't already name it. A folder that couldn't be read is listed as the saved folder. A long list scrolls on its own, from the keyboard too, and long names wrap inside the column on a narrow screen.GET /api/runtimealso returns how many saved entries couldn't be restored (restore_failures)./api/healthdoesn't change.registry_recipes_source, then reinitialize PyRIT or restart the backend. Other not-ready states keep the generic bar. The bar renders inside the layout, in the app's theme, and wraps its text, like the Configuration status bar.HuggingFaceChatTarget, it imports torch, and checks for CUDA when asked, in a worker thread, so the constructor doesn't hold the event loop.use_cudais read through its registered parameter first, so a textual "false" doesn't check for CUDA. Restore builds through the same create.redact_urls, so the cause stays in the log without the SAS query.pyrit_conf.mdsays that live apply needs one backend worker and one replica, and to restart otherwise.What stays the same:
GET /api/config/runtime(admin only) lists the failures with full messages, plus the ignored files, as in #3909. There's no separate list for everyone.Screenshots: Configuration at 390px wide before and after (the bar's wording, and the status and list inside the column), and the bar at 1024px.
Next: #4071 (part 20) adds Delete to each entry in this list.