Skip to content

Conversation

janeyx99
Copy link
Contributor

@janeyx99 janeyx99 commented Oct 7, 2020

Adding a sharding node to our python CONFIG_TREE

@janeyx99 janeyx99 changed the title attempt to add sharding option to test framework add sharding option to test framework Oct 7, 2020
@janeyx99 janeyx99 marked this pull request as ready for review October 7, 2020 21:56
@janeyx99 janeyx99 requested review from a team and malfet October 7, 2020 21:56
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

docker_image: "308535385114.dkr.ecr.us-east-1.amazonaws.com/pytorch/pytorch-linux-xenial-cuda9.2-cudnn7-py3-gcc7"
- pytorch_linux_test:
name: pytorch_linux_xenial_cuda9_2_cudnn7_py3_gcc7_test1
name: pytorch_linux_xenial_cuda9_2_cudnn7_py3_gcc7_test
Copy link
Contributor

Choose a reason for hiding this comment

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

is cu92 significantly faster than cuda10/11?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, but that particular test is not run in CI (not marked important in the CONFIG TREE)

Copy link
Contributor

@malfet malfet Oct 9, 2020

Choose a reason for hiding this comment

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

It is run in CI, but only on master (i.e. important tests run on both master and PRs, while others run only on master), but we don't need to shard it, as users usually do not wait for the test results on master.

@janeyx99 janeyx99 force-pushed the add-sharding-as-option branch from a5787f1 to 7741264 Compare October 8, 2020 20:40
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #45988   +/-   ##
=======================================
  Coverage   68.25%   68.25%           
=======================================
  Files         410      410           
  Lines       53246    53246           
=======================================
  Hits        36343    36343           
  Misses      16903    16903           

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 c8d76ff...7741264. Read the comment docs.

Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

LGTM

docker_image: "308535385114.dkr.ecr.us-east-1.amazonaws.com/pytorch/pytorch-linux-xenial-cuda9.2-cudnn7-py3-gcc7"
- pytorch_linux_test:
name: pytorch_linux_xenial_cuda9_2_cudnn7_py3_gcc7_test1
name: pytorch_linux_xenial_cuda9_2_cudnn7_py3_gcc7_test
Copy link
Contributor

@malfet malfet Oct 9, 2020

Choose a reason for hiding this comment

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

It is run in CI, but only on master (i.e. important tests run on both master and PRs, while others run only on master), but we don't need to shard it, as users usually do not wait for the test results on master.

@facebook-github-bot
Copy link
Contributor

@janeyx99 merged this pull request in 0983ddb.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants