-
Notifications
You must be signed in to change notification settings - Fork 813
UX: add progress indicators for translation tasks #1257
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
Merged
jmartin-tech
merged 3 commits into
NVIDIA:main
from
jmartin-tech:feature/multilingual-add-progress
Jun 27, 2025
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for ease of maintenance i'd like a central location for these bars, maybe within langproviders or langservice, so we can update them all in one go. should probably be invoked from the current calling locations. one change i'd like is the name of the langprovider being used, to be in the description, for consistency - this seems easier to do w/ a centralised def. a tqdm object might do the trick.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are
tqdmobjects already.I disagree with having the
langprovider's name such aslangprovider.remote.RivaTranslatorin the description here, how does that improve the user's context on progress of the run? I could see some value in inserting the probe name since that is not displayed yet when this progress bar starts.These should eventually be managed via a display or console presentation layer, I suggest we defer consolidation until we can build out a more consistent pattern.
The
callbackpattern used distinctly decouples the action taken from thenotificationthat the service is triggering to make this easier to refactor later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, correct me if I'm wrong, progress bars in garak are typically labelled with a description of the class that's running and maybe some salient config too.
If we put in the probe name, do we give the experience of seeing a bar with a probe name complete while language provisioned, and then another bar with similar description start from zero again while prompts are sent to the generator?
The pattern in this PR is pretty consistent, but repeated - can you say more about the expected level of consistency?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consolidation of all the scattered
tqdmusage into a display managing layer. The repeated pattern here is just two method calls, one toinstantiatethe bar and one to close it.Existing progress bars often refer to the plugin that creates them or caused them to be created, in this case I can see mimic of how a
tqdmfor a buff that is mutating prompts is labeled:Translation is a very similar action so I could see expanding the specificity of this tqdm to be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are seven very similar
tqdm.tqdmcalls in this PR all covering language provision. My question is, ist this the time to establish a pattern for encapsulating these, given that style & descriptions are likely to be very similar also? Doesn't need to block, current code just looks a bit far from DRYThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all
onemethod call that would need similar params passed to a new method, in my opinion this does to not make sense to refactor for this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aight. It does feel marginal - almost all the params are things that one would specify to an encapsulation anyway. Will send descr & RGB const commits