Skip to content
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

Qualx model updates from weekly KPI run 2025-01-10 #1495

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

nvauto
Copy link
Collaborator

@nvauto nvauto commented Jan 10, 2025

Description:

The latest /ssd0/qual/spark-rapids-tools-private/qual-kpis/kpi_summary_xgboost-2025-01-10.csv:

platform,tp_count,fp_count,tn_count,fn_count,precision,recall

databricks-aws,15,8,26,1,65.22,93.75

databricks-aws_photon,10,18,18,4,35.71,71.43

databricks-azure,18,1,21,8,94.74,69.23

databricks-azure_photon,20,7,6,15,74.07,57.14

dataproc,34,20,29,2,62.96,94.44

emr,13,9,25,4,59.09,76.47

onprem,27,19,28,2,58.7,93.1

Description:\n\nThe latest /ssd0/qual/spark-rapids-tools-private/qual-kpis/kpi_summary_xgboost-2025-01-10.csv:\n\nplatform,tp_count,fp_count,tn_count,fn_count,precision,recall\n\ndatabricks-aws,15,8,26,1,65.22,93.75\n\ndatabricks-aws_photon,10,18,18,4,35.71,71.43\n\ndatabricks-azure,18,1,21,8,94.74,69.23\n\ndatabricks-azure_photon,20,7,6,15,74.07,57.14\n\ndataproc,34,20,29,2,62.96,94.44\n\nemr,13,9,25,4,59.09,76.47\n\nonprem,27,19,28,2,58.7,93.1
Signed-off-by: nvauto <[email protected]>
@mattahrens
Copy link
Collaborator

KPIs look good on the updates....only minor changes is one more TN and one less FP for dataproc and onprem (which is good).

@leewyang any concerns on your side?

@amahussein amahussein added the user_tools Scope the wrapper module running CSP, QualX, and reports (python) label Jan 10, 2025
@leewyang
Copy link
Collaborator

It's interesting that while only dataproc and onprem had KPI deltas, all of the model binaries were different than before.

Since there were no dataset changes, these changes were likely due to:

  1. the code commits since the last model build, i.e. presumably this PR?
  2. build environment differences.

For (2), I had been using a fairly fixed conda environment on a dev box, so I think building from a controlled CI/CD environment is actually better. Note that I had seen some binary deltas when building models in different environments before, hence the use of a fixed conda env.

I will run the old process today just to confirm the expected deltas for dataproc and onprem and see if the other models change as well (or if they stay the same for my build environment).

@parthosa
Copy link
Collaborator

Question on this CI pipeline: Can we use a bash script in the pipeline that converts CSV to markdown format so that it renders better in Github?

@leewyang
Copy link
Collaborator

OK, finished running through the old process and comparing results.

  • All models did, in fact, change due to the recent code changes, but the model metrics were very similar to before (since the changes were minor). Note that my models were still slightly different.
  • For the KPI deltas, it looks like they were mostly due to data points that were very close to the threshold line, which subsequently moved across the line slightly one way or the other.

So, basically, I think these models would be fine to merge now.

Ideally, it would be nice to also auto-generate a PR for the evaluate metrics for the internal repo, but I think those are more of a nice-to-have at this point, since we can theoretically generate them offline, if needed.

@mattahrens should we address @parthosa's comment first before committing these models?

@mattahrens
Copy link
Collaborator

Question on this CI pipeline: Can we use a bash script in the pipeline that converts CSV to markdown format so that it renders better in Github?

Did you specifically mean the .metrics files as changed in this PR? We can do that separately.

@parthosa
Copy link
Collaborator

We can address this in the next weekly update as it is more of a cosmetic feature.

@mattahrens
Copy link
Collaborator

@mattahrens should we address @parthosa's comment first before committing these models?

No need to block this PR to make that change.

@leewyang
Copy link
Collaborator

Also, one other note. The script doesn't build/evaluate the combined model, but that's not really used, so again, it's something we could always do offline if needed.

@leewyang leewyang merged commit dd0b336 into dev Jan 10, 2025
14 checks passed
@leewyang leewyang deleted the qualx-model-update-weekly-2025-01-10 branch January 10, 2025 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user_tools Scope the wrapper module running CSP, QualX, and reports (python)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants