-
Notifications
You must be signed in to change notification settings - Fork 50
fix: configurator bugs fixes #640
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
Conversation
…ange it was not set up to True anywhere
…vailable through configurator.
… in case s2s was installed and ros2 was not sourced, it was logged that s2s is not installed
a15eb05
to
5d207dd
Compare
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.
Left a comment. The rest of the changes look good, thanks for fixing these.
@@ -975,29 +976,30 @@ def setup_steps(): | |||
step_names = ["👋 Welcome", "🤖 Model Selection", "📊 Tracing"] | |||
step_render = [welcome, model_selection, tracing] | |||
|
|||
st.session_state.features["s2s"] = True |
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.
Shall we add else to if
block below and move this line there ?
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.
Thank you for the comment. If - else statement does not change behavior of this functionality (in case s2s
is not installed, s2s
key would be set to False in the first try-except block), but the change suggested by you makes code more readable. I applied your suggestion and additionaly moved try-except statements to else block - to avoid triple warnings in case s2s
is not installed. 367e3b3
…ancy - previously, if s2s is not installed, the warning would occur 3 times
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.
LGTM.
Purpose
KeyError
excetpion duringTest Configuration
#633 and other bugs discovered during work with the configurator.Proposed Changes
KeyError
excetpion duringTest Configuration
#633 by setting up the ["s2s"] feature to True if rai_s2s is imported with success.Issues
KeyError
excetpion duringTest Configuration
#633Testing
KeyError
excetpion duringTest Configuration
#633s2s
installed