ci: replace make -C tools with devcluster. #2892

ioga · 2021-08-31T21:57:50Z

Description

Replace the last few parts of circleci that use make -C tools with devcluster.
Remove make -C tools run.

Test Plan

CI is green.

Commentary (optional)

Checklist

User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2021-08-31T21:57:55Z

✔️ Deploy Preview for determined-ui canceled.

🔨 Explore the source changes: 853db7f

🔍 Inspect the deploy log: https://app.netlify.com/sites/determined-ui/deploys/613965919ba16b00075a0ffc

rb-determined-ai · 2021-09-01T15:53:14Z

tools/Makefile

@@ -28,10 +14,6 @@ prep-root:
 	@ln -s $(abspath ../webui/react/build/*) build/webui/react
 	@ln -s $(abspath ../harness/dist/*.whl) build/wheels/

-.PHONY: run


Why not leave make -C tools run intact for a while? I think rigidly forcing devs to transition from one tooling system to another one is an unnecessary burden.

I think there should be at least a couple months after I land make devcluster and before we remove make -C tools run

hamidzr · 2021-09-01T22:57:35Z

sorta related: there is one more place we have a copy of the tools dir and that's used for running the tests locally. in webui/tests/test-cluster. We can leave that for another PR

ioga · 2021-09-01T23:07:18Z

sorta related: there is one more place we have a copy of the tools dir and that's used for running the tests locally. in webui/tests/test-cluster. We can leave that for another PR

Alright, I want get rid of it in this PR, so we are done with this.

hkang1 · 2021-09-02T17:41:30Z

sorta related: there is one more place we have a copy of the tools dir and that's used for running the tests locally. in webui/tests/test-cluster. We can leave that for another PR

Alright, I want get rid of it in this PR, so we are done with this.

🙏 ditto on this

hkang1

very excited for this work to land (love the branch name :))

ioga · 2021-09-03T00:53:37Z

@hamidzr @hkang1
Okay, I did the webui/tests/test-cluster. Can you please check out this PR and try running it the way you use it on your systems? There was some wild path and env magic in e2e-tests.py I had to update. I am not super sure about it.

hamidzr

it's great to see that we don't need to depend on package-push job before running e2e tests.
the main way I run e2e tests locally is through make test or python ./bin/e2e-tests.py e2e-tests .
there seems to be some issues with the devcluster config. it's complaining about db.image_name.

also, do I need to set an env variable? DOCKER_LOCALHOST? it'd be nice if the user could run local tests without having to figure out their docker networking settings.

python ./bin/e2e-tests.py e2e-tests
setting up the cluster..
--oneshot is only supported in server mode
Traceback (most recent call last):
  File "./bin/e2e-tests.py", line 93, in det_cluster
    cluster_process = setup_cluster(config, f)
  File "./bin/e2e-tests.py", line 78, in setup_cluster
    raise Exception(f"cluster {config['DET_MASTER']} is unreachable")
Exception: cluster http://localhost:8081 is unreachable

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./bin/e2e-tests.py", line 203, in <module>
    main()
  File "./bin/e2e-tests.py", line 199, in main
    fn(config)
  File "./bin/e2e-tests.py", line 130, in e2e_tests
    with det_cluster(config):
  File "/usr/lib/python3.6/contextlib.py", line 81, in __enter__
    return next(self.gen)
  File "./bin/e2e-tests.py", line 96, in det_cluster
    teardown_cluster(config, cluster_process.pid)
UnboundLocalError: local variable 'cluster_process' referenced before assignment
make: *** [Makefile:19: test] Error 1
Time: 0h:01m:17s

hamidzr · 2021-09-03T17:56:02Z

webui/tests/bin/e2e-tests.py

@@ -158,12 +153,9 @@ def get_config(args):
    config["CLUSTER_NAME"] = f"det_test_{args.det_port}"
    config["DET_MASTER"] = f"{args.det_host}:{args.det_port}"

-    env = {}


question: is this just for simplifying code or do we need to make this change? the idea is to minimize the effect of env variables when running processes through this.

devcluster rebuilds the master so it's up-to-date; that needs GOPATH/GOBIN setup properly. This means we'd need to add some new variables to make it work, but everyone's setup is different.

My idea is that if you can run devcluster, this test can just use the same environment and also work.

If some env variables are explicitly harmful, maybe we should just filter them out?

I'm thinking that being explicit about them could minimize potential surprises that we get but either way works

ioga · 2021-09-03T18:43:24Z

it's great to see that we don't need to depend on package-push job before running e2e tests.
the main way I run e2e tests locally is through make test or python ./bin/e2e-tests.py e2e-tests .
there seems to be some issues with the devcluster config. it's complaining about db.image_name.

can you please update to a recent devcluster? it had a number of changes recently and I wonder if you're just on an old version.

also, do I need to set an env variable? DOCKER_LOCALHOST? it'd be nice if the user could run local tests without having to figure out their docker networking settings.

I never heard about DOCKER_LOCALHOST so not sure what you're asking about. This thing needs devcluster and docker working, but shouldn't require any other advanced setup.

hamidzr · 2021-09-07T18:12:23Z

can you please update to a recent devcluster? it had a number of changes recently and I wonder if you're just on an old version.

let me do that.

the DOCKER_LOCALHOST comes from here I asked that because I remember as part of the devcluster setup I needed to provide it with the interface IP to my docker bridge which is something specific to user setup.

ioga · 2021-09-07T18:15:27Z

ok, let me know.
that $DOCKER_LOCALHOST is devcluster's internal thing it does, basically just a configuration magic. it's not a docker thing. you don't need to set it explicitly in your env.

hamidzr · 2021-09-07T18:17:34Z

webui/tests/test-cluster/devcluster.yaml

+        agent_id: agent3
+        fluent:
+          port: 24226  # default value is 24224
+          container_name: determined-fluent-3


suggestion: pin the agent slot type to be CPU to that the tests run on the same* type of accelerator between local and ci slot_type: cpu

hamidzr

the WebUI e2e tests section looks good to me. You can't run the tests with node version > 14 but that's the same story on master branch as well

hamidzr · 2021-09-07T18:47:34Z

can you please update to a recent devcluster? it had a number of changes recently and I wonder if you're just on an old version.

I asked that because I remember as part of the devcluster setup I needed to provide it with the interface IP to my docker bridge

ftr updating helped. I'm guessing it's because of the newly added feature to guess the host --no-guess-host

hkang1

This is SOOOOOO much better. Especially the removal of make -C tools from webui/tests! devcluster and make -C webui/tests test worked great for me. Thanks @ioga!

(#8947)

cla-bot bot added the cla-signed label Aug 31, 2021

rb-determined-ai reviewed Sep 1, 2021

View reviewed changes

ioga force-pushed the rip-tools branch from 7e78f2f to 3b02453 Compare September 1, 2021 22:45

ioga changed the title ~~chore: replace make -C tools with devcluster.~~ ci: replace make -C tools with devcluster. Sep 1, 2021

ioga marked this pull request as ready for review September 1, 2021 22:47

ioga requested review from hamidzr and hkang1 September 1, 2021 22:47

ioga marked this pull request as draft September 1, 2021 23:07

hkang1 reviewed Sep 2, 2021

View reviewed changes

ioga force-pushed the rip-tools branch from 3b02453 to 7b4e96d Compare September 3, 2021 00:44

ioga marked this pull request as ready for review September 3, 2021 00:47

hamidzr reviewed Sep 3, 2021

View reviewed changes

hamidzr reviewed Sep 7, 2021

View reviewed changes

hamidzr approved these changes Sep 7, 2021

View reviewed changes

hkang1 approved these changes Sep 7, 2021

View reviewed changes

ioga force-pushed the rip-tools branch from c0af410 to 3818314 Compare September 8, 2021 18:23

ioga added 4 commits September 8, 2021 18:37

chore: replace make -C tools with devcluster.

fc526bc

wip: remove second copy of make -C tools.

e5f681b

wip

a014d95

slot_type: cpu

853db7f

ioga force-pushed the rip-tools branch from 3818314 to 853db7f Compare September 9, 2021 01:38

ioga merged commit 9ded187 into determined-ai:master Sep 9, 2021

ioga deleted the rip-tools branch September 9, 2021 03:43

dannysauer modified the milestones: 0.0.102, 0.17.0 Feb 6, 2024

jesse-amano-hpe added a commit that referenced this pull request Mar 4, 2024

amend contributing doc to point to correct make rule, as of #2892

4e067f6

jesse-amano-hpe mentioned this pull request Mar 4, 2024

chore: amend contributing doc to point to correct make rule #8947

Merged

4 tasks

jesse-amano-hpe added a commit that referenced this pull request Mar 4, 2024

chore: amend contributing doc to point to correct make rule, as of #2892

4d587ac

jesse-amano-hpe added a commit that referenced this pull request Mar 9, 2024

chore: amend contributing doc to point to correct make rule, as of #2892

4ae0987

(#8947)

maxrussell pushed a commit that referenced this pull request Mar 21, 2024

chore: amend contributing doc to point to correct make rule, as of #2892

0a88be0

(#8947)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: replace make -C tools with devcluster. #2892

ci: replace make -C tools with devcluster. #2892

ioga commented Aug 31, 2021 •

edited

Loading

netlify bot commented Aug 31, 2021 •

edited

Loading

rb-determined-ai Sep 1, 2021

hamidzr commented Sep 1, 2021

ioga commented Sep 1, 2021

hkang1 commented Sep 2, 2021

hkang1 left a comment

ioga commented Sep 3, 2021

hamidzr left a comment •

edited

Loading

hamidzr Sep 3, 2021

ioga Sep 3, 2021

hamidzr Sep 7, 2021 •

edited

Loading

ioga commented Sep 3, 2021

hamidzr commented Sep 7, 2021 •

edited

Loading

ioga commented Sep 7, 2021

hamidzr Sep 7, 2021

hamidzr left a comment

hamidzr commented Sep 7, 2021

hkang1 left a comment •

edited

Loading

ci: replace make -C tools with devcluster. #2892

ci: replace make -C tools with devcluster. #2892

Conversation

ioga commented Aug 31, 2021 • edited Loading

Description

Test Plan

Commentary (optional)

Checklist

netlify bot commented Aug 31, 2021 • edited Loading

rb-determined-ai Sep 1, 2021

Choose a reason for hiding this comment

hamidzr commented Sep 1, 2021

ioga commented Sep 1, 2021

hkang1 commented Sep 2, 2021

hkang1 left a comment

Choose a reason for hiding this comment

ioga commented Sep 3, 2021

hamidzr left a comment • edited Loading

Choose a reason for hiding this comment

hamidzr Sep 3, 2021

Choose a reason for hiding this comment

ioga Sep 3, 2021

Choose a reason for hiding this comment

hamidzr Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

ioga commented Sep 3, 2021

hamidzr commented Sep 7, 2021 • edited Loading

ioga commented Sep 7, 2021

hamidzr Sep 7, 2021

Choose a reason for hiding this comment

hamidzr left a comment

Choose a reason for hiding this comment

hamidzr commented Sep 7, 2021

hkang1 left a comment • edited Loading

Choose a reason for hiding this comment

ioga commented Aug 31, 2021 •

edited

Loading

netlify bot commented Aug 31, 2021 •

edited

Loading

hamidzr left a comment •

edited

Loading

hamidzr Sep 7, 2021 •

edited

Loading

hamidzr commented Sep 7, 2021 •

edited

Loading

hkang1 left a comment •

edited

Loading