Skip to content

Commit 674e11c

Browse files
kostmofacebook-github-bot
authored andcommitted
order caffe2 ubuntu configs contiguously (pytorch#17427)
Summary: This involves another purely cosmetic (ordering) change to the `config.yml` to facilitate simpler logic. Other changes: * add some review feedback as comments * exit with nonzero status on config.yml mismatch * produce a diagram for pytorch builds Pull Request resolved: pytorch#17427 Differential Revision: D14197618 Pulled By: kostmo fbshipit-source-id: 267439d3aa4c0a80801adcde2fa714268865900e
1 parent c106629 commit 674e11c

9 files changed

+279
-203
lines changed

.circleci/README.md

+10
Original file line numberDiff line numberDiff line change
@@ -24,3 +24,13 @@ Furthermore, consistency is enforced within the YAML config itself, by using a s
2424
multiple parts of the file.
2525

2626
See https://github.com/pytorch/pytorch/issues/17038
27+
28+
29+
Future direction
30+
----------------
31+
32+
### Declaring sparse config subsets
33+
See comment [here](https://github.com/pytorch/pytorch/pull/17323#pullrequestreview-206945747):
34+
35+
In contrast with a full recursive tree traversal of configuration dimensions,
36+
> in the future future I think we actually want to decrease our matrix somewhat and have only a few mostly-orthogonal builds that taste as many different features as possible on PRs, plus a more complete suite on every PR and maybe an almost full suite nightly/weekly (we don't have this yet). Specifying PR jobs in the future might be easier to read with an explicit list when we come to this.

.circleci/cimodel/binary_build_definitions.py

+4-4
Original file line numberDiff line numberDiff line change
@@ -91,18 +91,18 @@ def gen_yaml_tree(self, build_or_test):
9191
return d
9292

9393

94-
def get_root(smoke):
94+
def get_root(smoke, name):
9595

9696
return make_build_configs.TopLevelNode(
97-
"Builds",
97+
name,
9898
make_build_configs.CONFIG_TREE_DATA,
9999
smoke,
100100
)
101101

102102

103103
def gen_build_env_list(smoke):
104104

105-
root = get_root(smoke)
105+
root = get_root(smoke, "N/A")
106106
config_list = conf_tree.dfs(root)
107107

108108
newlist = []
@@ -214,7 +214,7 @@ def add_jobs_and_render(jobs_dict, toplevel_key, smoke, cron_schedule):
214214

215215
jobs_dict[toplevel_key] = d
216216

217-
graph = visualization.generate_graph(get_root(smoke))
217+
graph = visualization.generate_graph(get_root(smoke, toplevel_key))
218218
graph.draw(toplevel_key + "-config-dimensions.png", prog="twopi")
219219

220220

.circleci/cimodel/caffe2_build_definitions.py

+15-14
Original file line numberDiff line numberDiff line change
@@ -12,27 +12,22 @@
1212
DOCKER_IMAGE_VERSION = 248
1313

1414

15-
# TODO Sort the config.yml upstream so the ubuntu configs are contiguous
1615
CONFIG_HIERARCHY = [
16+
(Ver("ubuntu", "14.04"), [
17+
(Ver("gcc", "4.8"), ["py2"]),
18+
(Ver("gcc", "4.9"), ["py2"]),
19+
]),
1720
(Ver("ubuntu", "16.04"), [
21+
(Ver("cuda", "8.0"), ["py2"]),
1822
(Ver("cuda", "9.0"), [
23+
# TODO make explicit that this is a "secret TensorRT build"
24+
# (see https://github.com/pytorch/pytorch/pull/17323#discussion_r259446749)
1925
"py2",
2026
"cmake",
2127
]),
2228
(Ver("cuda", "9.1"), ["py2"]),
2329
(Ver("mkl"), ["py2"]),
24-
]),
25-
(Ver("ubuntu", "14.04"), [
26-
(Ver("gcc", "4.8"), ["py2"]),
27-
]),
28-
(Ver("ubuntu", "16.04"), [
2930
(Ver("gcc", "5"), ["onnx_py2"]),
30-
(Ver("cuda", "8.0"), ["py2"]),
31-
]),
32-
(Ver("ubuntu", "14.04"), [
33-
(Ver("gcc", "4.9"), ["py2"]),
34-
]),
35-
(Ver("ubuntu", "16.04"), [
3631
(Ver("clang", "3.8"), ["py2"]),
3732
(Ver("clang", "3.9"), ["py2"]),
3833
(Ver("clang", "7"), ["py2"]),
@@ -42,6 +37,8 @@
4237
(Ver("cuda", "9.0"), ["py2"]),
4338
]),
4439
(Ver("macos", "10.13"), [
40+
# TODO ios and system aren't related. system qualifies where the python comes
41+
# from (use the system python instead of homebrew or anaconda)
4542
(Ver("ios"), ["py2"]),
4643
(Ver("system"), ["py2"]),
4744
]),
@@ -65,6 +62,7 @@ def is_build_only(self):
6562
"android",
6663
] or self.get_platform() == "macos"
6764

65+
# TODO: Eventually we can probably just remove the cudnn7 everywhere.
6866
def get_cudnn_insertion(self):
6967

7068
omit = self.language == "onnx_py2" \
@@ -128,7 +126,7 @@ def gen_yaml_tree(self):
128126
tuples.append(("BUILD_IOS", miniutils.quote("1")))
129127

130128
if self.phase == "test":
131-
use_cuda_docker = str(self.compiler) not in ["mkl", "gcc4.8", "gcc5"]
129+
use_cuda_docker = self.compiler.name == "cuda"
132130
if use_cuda_docker:
133131
tuples.append(("USE_CUDA_DOCKER_RUNTIME", miniutils.quote("1")))
134132

@@ -139,6 +137,8 @@ def gen_yaml_tree(self):
139137
if not self.distro.name == "macos":
140138
tuples.append(("BUILD_ONLY", miniutils.quote("1")))
141139

140+
# TODO: not sure we need the distinction between system and homebrew anymore. Our python handling in cmake
141+
# and setuptools is more robust now than when we first had these.
142142
if self.distro.name == "macos":
143143
tuples.append(("PYTHON_INSTALLATION", miniutils.quote("system")))
144144
tuples.append(("PYTHON_VERSION", miniutils.quote("2")))
@@ -150,7 +150,7 @@ def gen_yaml_tree(self):
150150
])
151151

152152
if self.phase == "test":
153-
is_large = str(self.compiler) in ["mkl", "gcc4.8"] or self.language == "onnx_py2"
153+
is_large = self.compiler.name != "cuda"
154154

155155
resource_class = "large" if is_large else "gpu.medium"
156156
d["resource_class"] = resource_class
@@ -187,6 +187,7 @@ def get_caffe2_workflows():
187187
configs = gen_build_list()
188188

189189
# TODO Why don't we build this config?
190+
# See https://github.com/pytorch/pytorch/pull/17323#discussion_r259450540
190191
filtered_configs = filter(lambda x: not (str(x.distro) == "ubuntu14.04" and str(x.compiler) == "gcc4.9"), configs)
191192

192193
x = []

.circleci/cimodel/conf_tree.py

+1-5
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,7 @@ def __init__(self, parent, node_name):
2020
self.props = {}
2121

2222
def get_label(self):
23-
label = self.node_name
24-
if not label:
25-
# FIXME this shouldn't be necessary
26-
label = "<None>"
27-
return label
23+
return self.node_name
2824

2925
def get_children(self):
3026
return []

0 commit comments

Comments
 (0)