-
Notifications
You must be signed in to change notification settings - Fork 37
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
sync w/ internal repo; update models #1083
Conversation
Signed-off-by: Lee Yang <[email protected]>
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.
Thanks @leewyang. LGTM. Welcome to tools repo 🎉
Once things are stable, It might be helpful to work on a separate feature branch to skip GitHub workflows related to maven.
cc: @amahussein
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.
Thanks @leewyang
A couple of issues I raised in my comment.
if not toc_list: | ||
raise ValueError(f'No CSV files found for: {ds_name}') | ||
else: |
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.
Lets set the standard:
- raising an error: it means something is really bad. In that case do we fallback to legacy Speedup?
- Return a default DF with speedup 1.0: this is the case when the QualX says that there is no speedup because there are ceratin features, files/columns I cannot read/empty. Then the speedup is 1.0
For the case when it is "not toc_list", do we want to raise an error? or return 1.0 after logging that files were missing?
CC: @eordentlich and @parthosa
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.
@amahussein this should be a very rare case. Basically, the user has requested to load CSV files for a dataset, and we have found none at all for the entire dataset. Previously, we would just skip this dataset (failing silently), so added this code to raise it to the user's attention. Presumably, the legacy Speedup would also fail in this case too (or would it just return 1.0?).
node_level_supp['Exec Is Supported'] = ( | ||
node_level_supp['Exec Is Supported'] | ||
| node_level_supp['Exec Name'].apply( | ||
lambda x: any([x.startswith(nm) for nm in unsupported_overrides]) | ||
) | ||
| node_level_supp['Exec Name'].apply( | ||
lambda x: x.startswith('WholeStageCodegen') | ||
) |
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.
I thought that change was rolled back..Just double checking.
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.
Per @eordentlich's commit to internal repo, this is a workaround for #860.
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.
Let's merge that to unblock then followup later on whether to raise-error or not in subsequent changes.
This PR syncs the migrated QualX code with the latest code from the internal repo.
Changes
Test
Following CMDs have been tested:
Internal Usage: