[hailctl] Fix dependency installation for hailctl dataproc clusters #12510

daniel-goldstein · 2022-11-28T19:44:58Z

I broke this when I separated out the dependencies for hail into two layers:

hailtop dependencies
hail dependencies, which builds on top of the hailtop dependencies.

This fix does two things:

Use the full dependencies in 1 & 2
Use fully pinned dependencies when installing on clusters which seems better than using our wide-range dependencies. I left the install-deps and install-dev-deps as the normal requirements files as those are meant for development (I think?) but am happy to take opinions on whether we should use fully pinned deps there as well. I have so far been going by the rule of thumb of fully-pinned for CI and production environments, more lax rules for dev environments.

See here for additional context.

cc: @tpoterba, any idea why the test dataproc test succeeded?

tpoterba · 2022-11-28T20:01:09Z

test_dataproc only runs on deploys AFAIK

daniel-goldstein · 2022-11-28T20:04:36Z

Like a release? I think the terminology is a little overloaded. In services code we consider "deploy" as the CI pipeline that runs whenever main is updated.

daniel-goldstein · 2022-11-28T20:07:16Z

Ah I see, all these steps cut out early for non-release deploys. So we wouldn't have released a broken package which is good, but we'd still end up with a broken deploy, which is annoying.

tpoterba · 2022-11-28T21:02:30Z

Sorry about the incorrect terminology! Your conclusion is my understanding, yes.

danking · 2022-11-28T21:58:05Z

hail/Makefile

@@ -268,13 +268,13 @@ $(eval $(call ENV_VAR,cloud_base))
 $(eval $(call ENV_VAR,wheel_cloud_path))

 python/hailtop/hailctl/deploy.yaml: env/cloud_base env/wheel_cloud_path
-python/hailtop/hailctl/deploy.yaml: $(resources) python/requirements.txt
+python/hailtop/hailctl/deploy.yaml: $(resources) python/pinned-requirements.txt


this target does not exist.

Does it need to be? It's just a file in the repo

This seems like a foot-gun to me. Suppose I'm trying to experiment with a modified version of Hail. I've modified python/requirements.txt but I've not run generate-pip-lockfile. I run make install-hailctl thinking that this will incorporate my changes into a new wheel file which I can use to start cluster. In reality, my changes are ignored and the old pinned requirements are used.

I think we need a rule like:

python/pinned-requirements.txt: python/requirements.txt docker run --rm -it \ -v $(HAIL_HAIL_DIR):/hail \ python:3.7-slim \ /bin/bash -c "pip install pip-tools && cd /hail && \ pip-compile --upgrade python/requirements.txt \ --output-file=python/pinned-requirements.txt"

Or a rule like:

python/pinned-requirements.txt: generate-pip-lockfile

This latter rule will trigger generate-pip-lockfile to always be run if we depend on the pinned-requirements.txt which isn't great. I think the best option is to have a rule for each requirements file and to also have generate-pip-lockfile as a special fast path for developers who want to update all the lock files.

Or, if we want to avoid invoking docker thrice, we could do this:

python/pinned-requirements.txt: update-pip-lockfiles python/dev/pinned-requirements.txt: update-pip-lockfiles python/hailtop/pinned-requirements.txt: update-pip-lockfiles .PHONY: update-pip-lockfiles update-pip-lockfiles: python/requirements.txt python/dev/requirements.txt python/hailtop/requirements.txt ...

And then to forcibly regenerate you gotta use -B/--always-make

Thanks for the clarification, this makes a lot of sense! I will incorporate these changes.

danking · 2022-11-28T21:58:20Z

hail/Makefile

@@ -324,7 +324,7 @@ install: $(WHEEL)

 .PHONY: install-on-cluster
 install-on-cluster: $(WHEEL)


this target can run without generated pinned requirements files.

Not sure I understand entirely. Do you mean that install-on-cluster should use unpinned requirements or is this a concern about the target / dependencies of the target?

This is similar to my concerns above, there's an implicit dependency between python/requirements.txt and python/pinned-requirements.txt. At the very least we should error if the pinned-requirements.txt is older than the requirements.txt

vladsavelyev · 2022-11-28T23:16:43Z

Thank you! (this issue is blocking us now)

questions

danking · 2022-11-30T18:37:51Z

hail/Makefile

@@ -268,13 +268,13 @@ $(eval $(call ENV_VAR,cloud_base))
 $(eval $(call ENV_VAR,wheel_cloud_path))

 python/hailtop/hailctl/deploy.yaml: env/cloud_base env/wheel_cloud_path
-python/hailtop/hailctl/deploy.yaml: $(resources) python/requirements.txt
+python/hailtop/hailctl/deploy.yaml: $(resources) python/pinned-requirements.txt


This seems like a foot-gun to me. Suppose I'm trying to experiment with a modified version of Hail. I've modified python/requirements.txt but I've not run generate-pip-lockfile. I run make install-hailctl thinking that this will incorporate my changes into a new wheel file which I can use to start cluster. In reality, my changes are ignored and the old pinned requirements are used.

I think we need a rule like:

python/pinned-requirements.txt: python/requirements.txt docker run --rm -it \ -v $(HAIL_HAIL_DIR):/hail \ python:3.7-slim \ /bin/bash -c "pip install pip-tools && cd /hail && \ pip-compile --upgrade python/requirements.txt \ --output-file=python/pinned-requirements.txt"

Or a rule like:

python/pinned-requirements.txt: generate-pip-lockfile

This latter rule will trigger generate-pip-lockfile to always be run if we depend on the pinned-requirements.txt which isn't great. I think the best option is to have a rule for each requirements file and to also have generate-pip-lockfile as a special fast path for developers who want to update all the lock files.

danking · 2022-11-30T18:39:23Z

hail/Makefile

@@ -324,7 +324,7 @@ install: $(WHEEL)

 .PHONY: install-on-cluster
 install-on-cluster: $(WHEEL)


This is similar to my concerns above, there's an implicit dependency between python/requirements.txt and python/pinned-requirements.txt. At the very least we should error if the pinned-requirements.txt is older than the requirements.txt

…r if out of date

…n path in dataproc

daniel-goldstein · 2022-12-06T00:26:19Z

Because of the unpredictable way that git clone might realize the requirements files, I removed the pinned-requirements file as a dependency of the changed targets. In both cases, regenerating that file (either in CI as part of the deploy.yaml target or on a cluster for install-on-cluster) could cause a dataproc cluster running with untested dependency versions even if the requirements.txt files are unchanged. I do, however, require that the pinned-requirements files be compatible using the same check we do in CI

I performed the following manual testing:

Creating a dataproc cluster through hailctl dataproc start
ssh'ing into said cluster, cloning this branch and running make -C hail install-on-cluster to completion
Updating the requirements.txt file to something incompatible and successfully installing on cluster again with updated pinned requirements

However, I'm not sure I'm actually doing this right. I checked that in step 2 I was not regenerating any pinned-requirments files, but in step 3 make updated the pinned requirements without me telling it to, I'm guessing because of the wheel's dependence on PY_FILES and I changed the source under hail/python. So I don't entirely understand why I have this desirable result.

updates

danking · 2022-12-06T15:59:57Z

hail/Makefile

+	HAIL_HAIL_DIR=$(HAIL_HAIL_DIR) \
+	../generate_pip_lockfile.sh python/hailtop/requirements.txt python/hailtop/pinned-requirements.txt
+
+python/pinned-requirements.txt: python/hailtop/requirements.txt python/requirements.txt


why do we have dependencies on hailtop here and on hailtop and hail in the dev target?

python/requirements.txt includes those requirements from python/hailtop/requirements.txt, and the dev requirements are constrained by python/requirements.txt, so if any of these requirements files changed, it could affect the pinned-requirements.

danking · 2022-12-06T16:03:18Z

hail/Makefile

 	rm -f $@
 	echo "dataproc:" >> $@
 	for FILE in $(notdir $(resources)); do \
 	  echo "  $$FILE: $(cloud_base)/$$FILE" >> $@ || exit 1; done
 	echo "  wheel: $(wheel_cloud_path)" >> $@
-	echo "  pip_dependencies: $(shell cat python/requirements.txt | sed '/^[[:blank:]]*#/d;s/#.*//' | grep -v pyspark | tr "\n" "|||")" >> $@
+	printf "  pip_dependencies: " >> $@
+	$(PIP) install pip-tools && ../check_pip_requirements.sh python/requirements.txt python/pinned-requirements.txt


Do I understand correctly that this will solve dependencies using both files as constraints, then compare the solution to the pinned (second argument)? The idea being that if I add a new dependency, a new transitive dependency, or a conflicting version to the first file, it will trigger a diff failure when the solution is compared to the existing pinned file?

Removed the targets due to the reproducibility issues we discussed, but ensured that the targets that use the pinned requirements files check that they are still compatible with the requirements.txt files. I tested this by changing the requirements file (one by making an incompatible version change, one by adding a new dependency) and saw that the deploy.yaml make target failed and emitted a message to recreate the lockfiles

daniel-goldstein · 2022-12-09T18:25:34Z

@danking Sorry for the delay on this. Trying to move this along faster now to unblock the release

danking

Please follow up with change to target naming of check-pip-lockfiles

[hailctl] Fix dependency installation for hailctl dataproc clusters

43cca74

daniel-goldstein added the prio:high label Nov 28, 2022

daniel-goldstein assigned danking Nov 28, 2022

daniel-goldstein mentioned this pull request Nov 28, 2022

[hailgenetics] Make hailgenetics/hailtop image #12446

Merged

iris-garden mentioned this pull request Nov 28, 2022

[release] update changelog for 0.2.106 #12509

Merged

danking previously requested changes Nov 28, 2022

View reviewed changes

danking previously requested changes Nov 30, 2022

View reviewed changes

daniel-goldstein added 13 commits December 1, 2022 12:51

make the pinned requirements files actual targets

f8b1f4b

strip the spaces from each dependency

8a64aec

fix hailctl dataproc package installation

d8a9bbf

require the user regenerate pinned-requirements for install-on-cluste…

b01c93a

…r if out of date

install pip-tools

670e474

pip installing pip-tools puts it in the user local bin which is not o…

48ecdf7

…n path in dataproc

fix #s

eac76a8

just have the user sudo

54a4cdc

add permission again

d2accff

fix requirements parsing before installation

a74014d

put lockfile generation into a separate file

ba6097e

fix for dataproc

59bfc0e

remove dependency from deploy.yaml

fe61242

danking previously requested changes Dec 6, 2022

View reviewed changes

remove targets but ensure compatibility

72735ed

daniel-goldstein added 6 commits December 9, 2022 13:26

undo requirements test change

c20895e

fix requirements

9e985a1

Merge branch 'main' into fix-hailctl-dataproc

5f718ef

fix'

8dd7a46

add file to build.yaml

fb14d9c

add more inputs in build.yaml

7bbfa3c

danking approved these changes Dec 13, 2022

View reviewed changes

make singular

84da26a

danking merged commit 01c8099 into hail-is:main Dec 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[hailctl] Fix dependency installation for hailctl dataproc clusters #12510

[hailctl] Fix dependency installation for hailctl dataproc clusters #12510

daniel-goldstein commented Nov 28, 2022 •

edited

Loading

tpoterba commented Nov 28, 2022

daniel-goldstein commented Nov 28, 2022

daniel-goldstein commented Nov 28, 2022 •

edited

Loading

tpoterba commented Nov 28, 2022

danking Nov 28, 2022

daniel-goldstein Nov 29, 2022

danking Nov 30, 2022

danking Nov 30, 2022 •

edited

Loading

daniel-goldstein Nov 30, 2022

danking Nov 28, 2022

daniel-goldstein Nov 29, 2022

danking Nov 30, 2022

vladsavelyev commented Nov 28, 2022 •

edited

Loading

danking Nov 30, 2022

danking Nov 30, 2022

daniel-goldstein commented Dec 6, 2022 •

edited

Loading

danking Dec 6, 2022

daniel-goldstein Dec 6, 2022

danking Dec 6, 2022

daniel-goldstein Dec 6, 2022

daniel-goldstein commented Dec 9, 2022

danking left a comment

		@@ -324,7 +324,7 @@ install: $(WHEEL)

		.PHONY: install-on-cluster
		install-on-cluster: $(WHEEL)

[hailctl] Fix dependency installation for hailctl dataproc clusters #12510

[hailctl] Fix dependency installation for hailctl dataproc clusters #12510

Conversation

daniel-goldstein commented Nov 28, 2022 • edited Loading

tpoterba commented Nov 28, 2022

daniel-goldstein commented Nov 28, 2022

daniel-goldstein commented Nov 28, 2022 • edited Loading

tpoterba commented Nov 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danking Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladsavelyev commented Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-goldstein commented Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-goldstein commented Dec 9, 2022

danking left a comment

Choose a reason for hiding this comment

daniel-goldstein commented Nov 28, 2022 •

edited

Loading

daniel-goldstein commented Nov 28, 2022 •

edited

Loading

danking Nov 30, 2022 •

edited

Loading

vladsavelyev commented Nov 28, 2022 •

edited

Loading

daniel-goldstein commented Dec 6, 2022 •

edited

Loading