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

fix scheduled download tests #2706

Merged
merged 14 commits into from
Sep 28, 2020
Merged

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Sep 24, 2020

I've messed up the syntax for the workflow triggers in #2675. Thus, the CI does not even spin up. I think, it uses push as a default trigger, which is why we see some failed runs.


Edit: Unfortunately, I need to fix two more things:

  • Using an abstract base for the download tester is not working as expected: the abstract class is also run which expectedly fails, since the abstract methods are not implemented. Skipping this also skips all subclasses and thus no test is run. I've rewritten the collection logic to reflect that.
  • The issue generation is bugged for labels including a colon such as module: datasets. I've opened and issue. Depending on the response speed of the maintainer we either need to simply upgrade the action version or remove the label.

@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #2706 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2706   +/-   ##
=======================================
  Coverage   72.87%   72.87%           
=======================================
  Files          95       95           
  Lines        8237     8237           
  Branches     1285     1285           
=======================================
  Hits         6003     6003           
  Misses       1839     1839           
  Partials      395      395           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e7a4b1...99ddef3. Read the comment docs.

@pmeier pmeier changed the title fix triggers for scheduled workflow fix scheduled download tests Sep 24, 2020
return dict(argnames="url, md5", argvalues=argvalues, ids=ids)


def places365():
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To add new download tests for a dataset, simply implement a similar function and add it to L130 and optionally to L135.

@pmeier
Copy link
Collaborator Author

pmeier commented Sep 25, 2020

As reported in JasonEtco/create-an-issue#65 (comment) this issue most likely does not lie in the action itself, but rather in a dependency. Thus, the fix is probably going to take a while and I've implemented a workaround. This means that the automatically created issue will not be labeled module: datasets for now, but I've assigned myself anyway.

@pmeier
Copy link
Collaborator Author

pmeier commented Sep 25, 2020

After some further inspection the issue does not lie with https://github.com/JasonEtco/create-an-issue, but rather with our repository configuration. I suspect the GitHub Actions bot has no permission to open the issue.

As a workaround, I'm disabling the issue generation for now and re-enable it as soon as this is resolved. IMO we should merge this now to at least have the test although their success / failure has to be checked manually. Since I'm not able to debug this any further myself, I can't tell how long it takes until this will be completely resolved.

@pmeier pmeier requested a review from fmassa September 25, 2020 15:19
@pmeier
Copy link
Collaborator Author

pmeier commented Sep 25, 2020

@fmassa Test failure is unrelated.

@pmeier
Copy link
Collaborator Author

pmeier commented Sep 28, 2020

#2714 showed that the issue creation works as expected when performed from a scheduled workflow. Thus, the last commit re-enables it.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fmassa fmassa merged commit 898802f into pytorch:master Sep 28, 2020
@pmeier pmeier deleted the fix-workflow-syntax branch October 21, 2020 15:06
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* fix triggers for scheduled workflow

* more fix

* add missing repository checkout

* try fix label in template

* rewrite test infrastructure

* trigger issue generation

* try fix issue template

* try remove quotes

* remove buggy label

* try fix title

* cleanup

* add more test details

* reenable issue creation

Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* fix triggers for scheduled workflow

* more fix

* add missing repository checkout

* try fix label in template

* rewrite test infrastructure

* trigger issue generation

* try fix issue template

* try remove quotes

* remove buggy label

* try fix title

* cleanup

* add more test details

* reenable issue creation

Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
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

Successfully merging this pull request may close these issues.

2 participants