Skip to content

[compliance_checker] add wegihts_initialiazation rule #92

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

mwawrzos
Copy link
Contributor

@mwawrzos mwawrzos commented Mar 23, 2021

This is an update to the compliance checker that introduces tests against the rules described in #80.

Update for the following models is ready:

  • recommendation
  • object_detection
  • image_segmentation
  • rnn_speech_recognition (it was ready before)
  • language_model
  • single_stage_detector
  • image_classification
  • reinforcement

recommendation, objeobject_detection and image_segmentation
@github-actions
Copy link

github-actions bot commented Mar 23, 2021

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@johntran-nv
Copy link

@bitfort and @petermattson , what do you think is an appropriate approval list for this one? I believe most folks are already aware of this requirement as we talked about it in the SWG, and folks even brought it up in other topics, so I'm inclined to just accept, but I think it would be good to get some other eyes on it.

@petermattson
Copy link
Contributor

petermattson commented Apr 21, 2021 via email

@johntran-nv
Copy link

Good idea, Peter. I sent out an email to the training alias, and said that I'd come back and merge on Friday, if no one objects.

@emizan76
Copy link
Contributor

A couple of issues / questions just came up:

  1. Has weight initialization logging been added to the references? I see only RNNT and Unet3D supporting that. Or the reference implementations do not have to follow this rule? I see a log and compliance directory in the training repo but I do not know how actively they are maintained and which submission rules are followed by the references.
  2. Is there an assumption that tensor names should be the same as the reference, or the ones listed in this PR? If submissions use different tensor names then it is tedious work to make this logging.

@mwawrzos
Copy link
Contributor Author

Answering the questions:

  1. I was trying to do this way, so references are updated first, and the compliance checker is updated accordingly, but the submitters WG committee was busy with other topics, and there was no time to bring references update topic. @TheKanter said on one of the benchmark infra WG meetings, that contractors will be hired, and they can update references. Apart from that, most of the references are outdated now, some of them don't even support MLPerf logging. That is why I decided to propose changes this way.
  2. Tensor names were supposed to be straightforward. I'm open to change them. If some benchmark is described in too much detail, or too little detail, it is also subject to change. The intention is to make submission review easier, so finding sources initializing weights is simpler. Does tensor name change to reference-framework-like make it more acceptable?

@petermattson
Copy link
Contributor

petermattson commented Apr 22, 2021 via email

@johntran-nv
Copy link

That's a good suggestion, Peter. Let's defer this to v1.1, then. I'll leave it open.

@petermattson
Copy link
Contributor

petermattson commented Apr 23, 2021 via email

@xyhuang
Copy link
Contributor

xyhuang commented Sep 21, 2021

Fixed in #153

@xyhuang xyhuang closed this Sep 21, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants