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

Proposal for improved submission process #61

Closed
georgelyuan opened this issue Sep 21, 2020 · 34 comments
Closed

Proposal for improved submission process #61

georgelyuan opened this issue Sep 21, 2020 · 34 comments
Assignees

Comments

@georgelyuan
Copy link
Contributor

The current submission process is inadequate in two ways:

  1. It allows submitters who have not yet submitted to access the submission repo and preview other submitters' results. This gives potential submitters an unfair competitive advantage if they wait until the last minute before deciding to submit. Unfairness is further compounded by the submission deadline not being friendly for all time zones. Companies in Asia typically submit the night before. Also, it is difficult for companies that coordinate with worldwide partners to synchronize submission time, putting them at a further disadvantage .
  2. It allows companies who have no interest in submitting early access to results.

Proposal for next round:

  • Submitters mirror the submission repo (https://docs.github.com/en/github/creating-cloning-and-archiving-repositories/duplicating-a-repository) and push the submission to their mirror. Before submission deadline, submitter must notify a designated non-submitting chair (Vijay/David?) of their intention to submit along with a link and access to their submission mirror.
  • After submission deadline, the non-submitting chairs will then merge all of the submission mirrors' contents into a new 'results_preview' mirror of the submission repo. This ensures submitters will not be able to preview other submitters' submissions.
  • After the 'results_preview' mirror has been populated, access is then simultaneously granted to only those submitters that have actually submitted.

@DilipSequeira @petermattson @TheKanter

@guschmue
Copy link
Contributor

+1. An alternative would be to upload a tgz to a write-only blob storage that mlperf folk would merge into the repo and give access to the repo only once the tgz is merged.

@petermattson
Copy link
Contributor

petermattson commented Sep 22, 2020 via email

@georgelyuan
Copy link
Contributor Author

Sure, personally I'm not hung up on how we do it, as long as we address the one-sided visibility issue.
I'm sure we can come up with something reasonable.

@TheKanter
Copy link
Contributor

AIs going forward, discuss in training, amend rules, change infrastructure.

@TheKanter
Copy link
Contributor

TheKanter commented Jan 5, 2021

Ashwin suggests the following:

  1. Submitter creates private fork of repo
  2. Populate private fork with submission
  3. By deadline, must submit (1) a link to the repo and (2) githash
  4. After deadline, submitter makes repo public, and chairs merge into official submission repo* (Official submission repo is accessible only by submitters)

Ashwin will investigate whether PRs from a private submitters repo can be sent to the private MLC submitters repo and report back.

We would like feedback from training, HPC, mobile on this.

@georgelyuan
Copy link
Contributor Author

To clarify step 4, if the PR is being made from a private fork to another private fork, I don't think the forks have to be made public, as long as the chair has access to both forks, but I guess that needs to be verified.
Perhaps step 4 should be reworded as:
4. After deadline, submitter gives access to chairs, who will merge into official submission repo* (Official submission repo will be made accessible only to submitters and only after all submissions have been merged)

@psyhtest
Copy link

psyhtest commented Jan 6, 2021

I kinda like the old approach, when you can submit your results as you generate them in the window between the random seeds being distributed and the submission deadline. It gives me a warm and fuzzy feeling having PRs piling up in the main submission repo. I don't mind in the slightest that other submitters can see them early.

Whatever else we decide to allow, can we keep the old approach please?

@psyhtest
Copy link

psyhtest commented Jan 6, 2021

It allows companies who have no interest in submitting early access to results.

We already guard against that somewhat, by asking non-submitters to declare themselves before the repo gets properly populated (e.g. 1 week). Of course, a submitter can still declare they intend to submit until the last minute and then not to, or submit and then withdraw. Were the review committee to suspect dishonest behaviour, they could take a vote to disqualify them from submitting to the next submission round.

@guschmue
Copy link
Contributor

guschmue commented Jan 6, 2021

The proposal is just to allow delaying the real PR by providing a hash in the meantime. If you send the PR as you would have in the past it should be the same imo.

@georgelyuan
Copy link
Contributor Author

georgelyuan commented Jan 6, 2021

I think it's fine if one wants to directly submit their PR to the submission repo. Note however that this submission repo is accessible by all potential submitters, including those who in the end may not actually submit. One of the goals of this proposal is to prevent such 'no-show' submitters from gaining access to the results before the general public.
In order to do that, I think the results chairs would have to merge submissions from the original submission repo and any submitter forks to another fork that is access-restricted to real submitters.

So to clarify what I have in mind:
draft_submission_repo - public repo that submitters can fork to push their results, or directly submit to if they so choose. used for CLA checks, uploading calibration docs. accessible by all potential submitters.
[company]_submission_repo - private forks of the draft submission repo that various submitters will use to make their submission. owned by the submitter, who will give access to the results chairs by submission deadline.
final_submission_repo - private fork of the draft submission repo that will contain merges of all submissions in the draft_submission_repo and _submission_repos. owned by the results chair, who will give access to valid submitters + other chairs.

After the final_submission_repo has been populated, the draft_submission_repo can probably be deleted.

@ashwin
Copy link

ashwin commented Jan 12, 2021

I found that Github does NOT allow these operations:

  • Creating a private fork from a public repo.
  • Creating a private branch on a public repo.
  • Giving a pull request to repo1 from a repo2, where repo2 is a clone (not a fork) of repo1. That is, repo2 has to be officially created as a "Github fork" of repo1 for PRs to work.

@georgelyuan Looks like the steps need to be:

  1. Clone (not fork) a "private repo" copy of the public repo. We share this "private repo" to all submitters.
  2. Submitters make forks of the private repo, work on their submissions there and submit their PRs at the submission deadline hour. We could request submitters to put up the git hash of their final submission at the deadline hour to the PR description/comment or someplace which has history tracking. Note that all submitters would be able to see each others' results in the duration between submission and going public.
  3. On publication day (or before), merge all the PRs to 1.0 branch in the private repo. Push that branch from private repo to the public repo, thus making the results public for the first time.

@guschmue
Copy link
Contributor

For the submission repo we never have a public repo. What gets published at the end is a copy into a new repo. This is because we don't want to have the issues filed during review go public and we need to avoid that revoked submissions (that are still in that repo) become public.

Not totally sure what happens between step 1 and step 2 above - how does the submitters repo gets PRed back into our private repo ? I think as long submitters repo is private the PR will not work cross org.

One could make a git patch and upload that to some write only blob storage at the deadline and we'd just apply that patch to the repo in mlcommons.

@ashwin
Copy link

ashwin commented Jan 12, 2021

@guschmue Aha, I did not realize PRs do not work between private forks. Your git patch seems like a feasible solution. What is an example of a write only blob storage space?

@guschmue
Copy link
Contributor

aws, azure, gcp have blob storage and you can create write only access (I know for sure on azure but aws and gcp will do the same)

@christ1ne
Copy link
Contributor

@georgelyuan please work with Guenther to have a workable solution. Please note it is optional for the submitters. The old way of submitting is fine for v1.0

@DilipSequeira
Copy link
Contributor

DilipSequeira commented Jan 26, 2021

Revised proposal:

By the submission deadline, submitter will submit 4 things to the results chair to show Proof of Work:

  • A URL to the location in public blob storage of an encrypted tarball containing the open/* and closed/* submission files. The blob storage can be one that is set up by MLPerf beforehand specifically for the submission or any public blob storage, as long as downloading can be automated (i.e. with curl)
  • A password to the encrypted tarball
  • A sha1sum of the tarball
  • The submission checker output (as cursory evidence that the submission is valid)

As soon as the deadline passes, the submission repository goes private. Access will be removed for everyone EXCEPT the following parties:

  • Chairs
  • Submitters who submitted valid submissions using the original submission process
  • Submitters who have submitted Proof of Work using the new submission process that have been verified by the results chair to contain a valid submission.

For #3 above, verification will be performed by the results chair using a script (NVIDIA can contribute to this):

  • add_submission.py -u <submission URL> -p <password> -c <submitter company> -t <submission repo directory> -g <submitter github handles> -h <sha1 hash>
  • Downloads the encrypted submission tarball into submission repo directory
  • Verify tarball hash matches provided hash
  • Decrypts tarball using password
  • Run submission checker to verify submission looks OK
  • Give results repo access to submitter github handles (not sure this can be done on the commandline? If not, step 6 will be a separate script)
  • Git add, commit, branch, push the files in /closed/ and /open/

With the process outlined, the results chair will push submissions made using the new secure submission process to minimize delays. The competitive intelligence gap raised by the issue now no longer exists since no participant can now view secure submissions before the deadline (submission tarballs are encrypted). Once the deadline has passed, only verified submitters will be able to view each other’s submissions.

In an alternative post-deadline process, the chair can give access to the submitter and have the submitter push the results in a timely manner. At this point, the submitter is committed to submitting results. If they do not for whatever reason, then the results chair will do so in their stead, since by that point the submitter will have access to other submitted submissions. Once the submitter pushes the results, the results chair will still need to verify that the submission matches the decrypted extracted tarball.

@christ1ne
Copy link
Contributor

WG: submission rules will be updated with this optional process. PR review next meeting.

@christ1ne
Copy link
Contributor

@guschmue @georgelyuan are working on the script for the submission encryption. Pending script check-in.

submitter can pick any storage but the access has to be public.

@christ1ne
Copy link
Contributor

@georgelyuan PR reminder :)

@georgelyuan
Copy link
Contributor Author

Should I just put it in the MLCommons shared drive? Not sure where to put the script

@guschmue
Copy link
Contributor

I have a bunch of submission related scripts here:
https://github.com/mlcommons/inference/tree/master/tools/submission
you could add it there.

@georgelyuan
Copy link
Contributor Author

Right my only objection is that the script is not inference-specific..
Do you think that's okay?

@guschmue
Copy link
Contributor

yeah, no worry - we can come up with something better once training folks ask for it.

@georgelyuan
Copy link
Contributor Author

@guschmue do you have a mlcommons email? Or should I instruct folks to use your microsoft email?

@guschmue
Copy link
Contributor

good question, maybe we should create a new address for this. Let me find somebody to create one.

@christ1ne
Copy link
Contributor

any linked PR for this? @georgelyuan

@guschmue
Copy link
Contributor

pending on me - I'm waiting for Peter to create mail alias for it

@christ1ne
Copy link
Contributor

@petermattson

@guschmue
Copy link
Contributor

@georgelyuan ... you can use submissions@mlcommons.org

@georgelyuan
Copy link
Contributor Author

D'oh looks like I'm missing the mlcommons CLA or something? I just submitted my form.

@georgelyuan
Copy link
Contributor Author

Ah I see Guenther had sent me an invitation back in January but it expired :(
Can you send it again? Thanks!

georgelyuan added a commit to georgelyuan/inference that referenced this issue Feb 23, 2021
@georgelyuan
Copy link
Contributor Author

mlcommons/inference#846
Hopefully I did this right. It's been a while..

guschmue pushed a commit to mlcommons/inference that referenced this issue Feb 23, 2021
@christ1ne
Copy link
Contributor

@georgelyuan is there any rule text update related to this? Thanks for the script.

@georgelyuan
Copy link
Contributor Author

#71

guschmue added a commit to mlcommons/inference that referenced this issue Feb 24, 2021
* fix typo

* Add missing return statement (#839)

Broken by #779.

* Add missing letters (#843)

Co-authored-by: christ1ne <christine.cheng@intel.com>

* submission packing script for mlcommons/policies#61 (#846)

* Update Seeds to 1.0 (#847)

* fix docker images (#844)

Co-authored-by: christ1ne <christine.cheng@intel.com>

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>
Co-authored-by: tjablin <tjablin@google.com>
Co-authored-by: georgelyuan <53881988+georgelyuan@users.noreply.github.com>
guschmue added a commit to mlcommons/inference that referenced this issue Apr 6, 2021
* pull changes from master to r1.0 (#851)

* fix typo

* Add missing return statement (#839)

Broken by #779.

* Add missing letters (#843)

Co-authored-by: christ1ne <christine.cheng@intel.com>

* submission packing script for mlcommons/policies#61 (#846)

* Update Seeds to 1.0 (#847)

* fix docker images (#844)

Co-authored-by: christ1ne <christine.cheng@intel.com>

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>
Co-authored-by: tjablin <tjablin@google.com>
Co-authored-by: georgelyuan <53881988+georgelyuan@users.noreply.github.com>

* Handle NaN/Inf correctly in LoadGen Logger (#837)

* Handle NaN/Inf correctly in LoadGen Logger

* Initialize target_latency_percentile in Offline scenario to avoid NaNs

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Add system description JSON fields for power submissions (#853)

* Check and parse power logs in submission checker (#854)

* Check and parse power logs in submission checker

* fixup required_perf_files in submission checker

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Print out correct error message when seeds are wrong (#858)

* take out the master content (#859)

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Fix links to benchmark and update submission date (#868)

* Fix message when performance_sample_count is too small (#865)

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Fix SingleStream latency metric in submission checker (#872)

* Add Power WG's check.py checking to submission checker (#873)

* Add Power WG's check.py checking to submission checker

* Update power-dev submodule to latest r1.0

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Update Power WG check.py to latest r1.0 (#884)

* ignore .github directory (#881)

* Change power metric to J/inference for SingleStream/MultiStream (#886)

* Change power metric to J/inference for SingleStream/MultiStream

* fix a spelling typo

* Fix power metric for SingleStream and update power-dev hash

* Fix typo

* fix url in results report (#890)

* Handle timezone differences when parsing power samples (#896)

* use host_processors_per_node in final report (#897)

* use host_processors_per_node in final report

* report per node processors

* fix typo

* New PyTorch ResNet50-v1.5 links (#876)

* New PyTorch ResNet50-v1.5 links

* Delete unused import

* Guard max_latency_ with latencies_mutex_ (#878)

* Guard max_latency_ with latencies_mutex_

There was a race in the computation of atomic max and std::memory_order_release
is not a valid order for atomic loads. The code isn't performance critical, so
it is easier and safer to use mutexes instead.

* Initialize max_latency_

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* more intuitive error message (#883)

we can just update the master for now

Co-authored-by: christ1ne <christine.cheng@intel.com>
Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>
Co-authored-by: georgelyuan <53881988+georgelyuan@users.noreply.github.com>
Co-authored-by: nvpohanh <53919306+nvpohanh@users.noreply.github.com>
Co-authored-by: kahmed10 <15948690+kahmed10@users.noreply.github.com>
Co-authored-by: Core.Halt <46298335+corehalt@users.noreply.github.com>
tjablin pushed a commit to mlcommons/inference that referenced this issue May 24, 2022
* submission packing script for mlcommons/policies#61

* loadgen and compliance test changes to support new TEST04

* fxing script errors

* lgtm checks

* lgtm checks v2

* lgtm checks v3

* Performance checker now looks at the early stop stats and server target latency

* edits from Anton
tjablin added a commit to mlcommons/inference that referenced this issue Jun 6, 2022
* submission packing script for mlcommons/policies#61

* loadgen and compliance test changes to support new TEST04

* fxing script errors

* lgtm checks

* lgtm checks v2

* lgtm checks v3

* Performance checker now looks at the early stop stats and server target latency

* edits from Anton

* fixing check_performance_dir args

Co-authored-by: tjablin <tjablin@google.com>
arjunsuresh pushed a commit to GATEOverflow/inference that referenced this issue Apr 29, 2024
arjunsuresh pushed a commit to GATEOverflow/inference that referenced this issue Apr 29, 2024
* pull changes from master to r1.0 (mlcommons#851)

* fix typo

* Add missing return statement (mlcommons#839)

Broken by mlcommons#779.

* Add missing letters (mlcommons#843)

Co-authored-by: christ1ne <christine.cheng@intel.com>

* submission packing script for mlcommons/policies#61 (mlcommons#846)

* Update Seeds to 1.0 (mlcommons#847)

* fix docker images (mlcommons#844)

Co-authored-by: christ1ne <christine.cheng@intel.com>

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>
Co-authored-by: tjablin <tjablin@google.com>
Co-authored-by: georgelyuan <53881988+georgelyuan@users.noreply.github.com>

* Handle NaN/Inf correctly in LoadGen Logger (mlcommons#837)

* Handle NaN/Inf correctly in LoadGen Logger

* Initialize target_latency_percentile in Offline scenario to avoid NaNs

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Add system description JSON fields for power submissions (mlcommons#853)

* Check and parse power logs in submission checker (mlcommons#854)

* Check and parse power logs in submission checker

* fixup required_perf_files in submission checker

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Print out correct error message when seeds are wrong (mlcommons#858)

* take out the master content (mlcommons#859)

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Fix links to benchmark and update submission date (mlcommons#868)

* Fix message when performance_sample_count is too small (mlcommons#865)

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Fix SingleStream latency metric in submission checker (mlcommons#872)

* Add Power WG's check.py checking to submission checker (mlcommons#873)

* Add Power WG's check.py checking to submission checker

* Update power-dev submodule to latest r1.0

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* Update Power WG check.py to latest r1.0 (mlcommons#884)

* ignore .github directory (mlcommons#881)

* Change power metric to J/inference for SingleStream/MultiStream (mlcommons#886)

* Change power metric to J/inference for SingleStream/MultiStream

* fix a spelling typo

* Fix power metric for SingleStream and update power-dev hash

* Fix typo

* fix url in results report (mlcommons#890)

* Handle timezone differences when parsing power samples (mlcommons#896)

* use host_processors_per_node in final report (mlcommons#897)

* use host_processors_per_node in final report

* report per node processors

* fix typo

* New PyTorch ResNet50-v1.5 links (mlcommons#876)

* New PyTorch ResNet50-v1.5 links

* Delete unused import

* Guard max_latency_ with latencies_mutex_ (mlcommons#878)

* Guard max_latency_ with latencies_mutex_

There was a race in the computation of atomic max and std::memory_order_release
is not a valid order for atomic loads. The code isn't performance critical, so
it is easier and safer to use mutexes instead.

* Initialize max_latency_

Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>

* more intuitive error message (mlcommons#883)

we can just update the master for now

Co-authored-by: christ1ne <christine.cheng@intel.com>
Co-authored-by: Guenther Schmuelling <guschmue@microsoft.com>
Co-authored-by: georgelyuan <53881988+georgelyuan@users.noreply.github.com>
Co-authored-by: nvpohanh <53919306+nvpohanh@users.noreply.github.com>
Co-authored-by: kahmed10 <15948690+kahmed10@users.noreply.github.com>
Co-authored-by: Core.Halt <46298335+corehalt@users.noreply.github.com>
Former-commit-id: 10aa90b
arjunsuresh pushed a commit to GATEOverflow/inference that referenced this issue Apr 29, 2024
* submission packing script for mlcommons/policies#61

* loadgen and compliance test changes to support new TEST04

* fxing script errors

* lgtm checks

* lgtm checks v2

* lgtm checks v3

* Performance checker now looks at the early stop stats and server target latency

* edits from Anton

Former-commit-id: e8e72cc
arjunsuresh pushed a commit to GATEOverflow/inference that referenced this issue Apr 29, 2024
* submission packing script for mlcommons/policies#61

* loadgen and compliance test changes to support new TEST04

* fxing script errors

* lgtm checks

* lgtm checks v2

* lgtm checks v3

* Performance checker now looks at the early stop stats and server target latency

* edits from Anton

* fixing check_performance_dir args

Co-authored-by: tjablin <tjablin@google.com>
Former-commit-id: 98776fd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants