Skip to content

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Sep 10, 2021

Fixes #64760

This should hopefully put the torchhub tests back.

This also avoids skipping the torchhub tests: currently the tests are skipped if they fail, which pretty much defeats the purpose of having a test in the first place since we're never notified when they do fail.

cc @ezyang @seemethere @malfet @lg20987 @pytorch/pytorch-dev-infra @nairbv @NicolasHug

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 10, 2021

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 2db3538 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@NicolasHug
Copy link
Member Author

According to https://github.com/pytorch/pytorch/pull/64807/checks?check_run_id=3565497882 the torchhub tests are properly passing now (i.e. they're not skipped).

CUSTOM_TEST_ARTIFACT_BUILD_DIR: build/custom_test_artifacts
ALPINE_IMAGE: "308535385114.dkr.ecr.us-east-1.amazonaws.com/tool/alpine"
PR_LABELS: ${{ toJson(github.event.pull_request.labels.*.name) }}
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only changed the linux template, I'm not sure the windows tests are running the torchhub tests. @seemethere would you know how I could confirm that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll also need to add -e GITHUB_TOKEN to the docker runs as well so that it gets propagated to the container as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually nvm, we already do this since we create an env file that gets auto-included in our docker runs:

- name: Preserve github env variables for use in docker
run: |
env | grep '^GITHUB' > "/tmp/github_env_${GITHUB_RUN_ID}"

@NicolasHug NicolasHug marked this pull request as ready for review September 10, 2021 10:39
@NicolasHug NicolasHug changed the title Pass GITHUB_TOKEN to linux CI jobs Pass GITHUB_TOKEN to linux CI jobs and avoid skipping torchhub tests Sep 10, 2021
@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #64807 (fb7e0ba) into master (f69cf3c) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #64807      +/-   ##
==========================================
+ Coverage   66.38%   66.46%   +0.08%     
==========================================
  Files         727      727              
  Lines       93573    93573              
==========================================
+ Hits        62117    62193      +76     
+ Misses      31456    31380      -76     

CUSTOM_TEST_ARTIFACT_BUILD_DIR: build/custom_test_artifacts
ALPINE_IMAGE: "308535385114.dkr.ecr.us-east-1.amazonaws.com/tool/alpine"
PR_LABELS: ${{ toJson(github.event.pull_request.labels.*.name) }}
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually nvm, we already do this since we create an env file that gets auto-included in our docker runs:

- name: Preserve github env variables for use in docker
run: |
env | grep '^GITHUB' > "/tmp/github_env_${GITHUB_RUN_ID}"

@seemethere seemethere added module: ci Related to continuous integration module: hub labels Sep 16, 2021
@facebook-github-bot
Copy link
Contributor

@NicolasHug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@NicolasHug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@NicolasHug merged this pull request in 9157a28.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla signed Merged module: ci Related to continuous integration module: hub

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Torchhub core tests are silently broken because of a Github API rate issue

3 participants