[httpx][Makefile][copier] add --timeout to copier; use in Makefile #14138

danking · 2024-01-11T17:28:05Z

We have long had trouble with copying on flaky Internet connections. AFAIK, this is due to our very aggressive timeouts. This change permits the CLI to override the default timeout in hailtop.aiotools.copy. Setting this back to 600s should work on most flaky Internet connections. It works on my particularly bad home WiFi.

The upload-docs target was old and broken so I deleted it. I did not change the upload-artifacts target because that is Google Cloud specific anyway.

Main benefit is that these targets work in Azure now.

We have long had trouble with copying on flaky Internet connections. AFAIK, this is due to our very aggressive timeouts. This change permits the CLI to override the default timeout in `hailtop.aiotools.copy`. Setting this back to 600s should work on most flaky Internet connections. It works on my particularly bad home WiFi. The upload-docs target was old and broken so I deleted it. I did not change the upload-artifacts target because that is Google Cloud specific anyway.

danking · 2024-01-11T17:28:51Z

Sent this to you, Ed, since, IIRC, you experienced broken uploads in the past due to our bad copy implementation. If you could clone this and try

rm hail/upload-remote-test-resources && make -C hail upload-remote-test-resources

that'd be most appreciated!

ehigham · 2024-01-11T18:21:47Z

From a fresh clone, the above (modified with rm -f) fails with:

$ rm -f hail/upload-remote-test-resources && make -C hail upload-remote-test-resources
make: Entering directory '/home/edmund/.local/src/hail/hail'
# # If hailtop.aiotools.copy gives you trouble:
# gcloud storage cp -r src/test/resources/\* gs://hail-test-ezlis/edmund/hail-test-resources/test/resources/
# gcloud storage cp -r python/hail/docs/data/\* gs://hail-test-ezlis/edmund/hail-test-resources/doctest/data/
python3 -m hailtop.aiotools.copy -vvv 'null' '[\
             {"from":"src/test/resources","to":"gs://hail-test-ezlis/edmund/hail-test-resources/test/resources/"},\
             {"from":"python/hail/docs/data","to":"gs://hail-test-ezlis/edmund/hail-test-resources/doctest/data/"}\
        ]' --timeout 600
/home/edmund/.local/src/hail/.venv/bin/python3: Error while finding module specification for 'hailtop.aiotools.copy' (ModuleNotFoundError: No module named 'hailtop')
make: *** [Makefile:355: upload-remote-test-resources] Error 1
make: Leaving directory '/home/edmund/.local/src/hail/hail'

I'll try again with hailtop installed - just wanted to point out the dependency failure in Makefile.

ehigham · 2024-01-11T18:36:14Z

With hailtop installed, I get:

$ rm -f hail/upload-remote-test-resources && make -C hail upload-remote-test-resources                                                  
make: Entering directory '/home/edmund/.local/src/hail/hail'
# # If hailtop.aiotools.copy gives you trouble:
# gcloud storage cp -r src/test/resources/\* gs://hail-test-ezlis/edmund/hail-test-resources/test/resources/
# gcloud storage cp -r python/hail/docs/data/\* gs://hail-test-ezlis/edmund/hail-test-resources/doctest/data/
python3 -m hailtop.aiotools.copy -vvv 'null' '[\
             {"from":"src/test/resources","to":"gs://hail-test-ezlis/edmund/hail-test-resources/test/resources/"},\
             {"from":"python/hail/docs/data","to":"gs://hail-test-ezlis/edmund/hail-test-resources/doctest/data/"}\
        ]' --timeout 600
Traceback (most recent call last):
  File "/home/edmund/.pyenv/versions/3.9.17/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/home/edmund/.pyenv/versions/3.9.17/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/edmund/.local/src/hail/hail/python/hailtop/aiotools/copy.py", line 211, in <module>
    asyncio.run(main())
  File "/home/edmund/.pyenv/versions/3.9.17/lib/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "uvloop/loop.pyx", line 1517, in uvloop.loop.Loop.run_until_complete
  File "/home/edmund/.local/src/hail/hail/python/hailtop/aiotools/copy.py", line 182, in main
    files = json.loads(args.files)
  File "/home/edmund/.pyenv/versions/3.9.17/lib/python3.9/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/home/edmund/.pyenv/versions/3.9.17/lib/python3.9/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/home/edmund/.pyenv/versions/3.9.17/lib/python3.9/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 2 (char 1)
make: *** [Makefile:355: upload-remote-test-resources] Error 1
make: Leaving directory '/home/edmund/.local/src/hail/hail'

What am I missing?

danking · 2024-01-12T18:34:03Z

@ehigham make --version? This is me:

GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

This program built for i386-apple-darwin11.3.0

danking · 2024-01-12T18:39:26Z

In particular, it appears that your Make might be invoking bash with the sixth argument starting with the three characters '[\. My Make sends a string whose first three characters are '[ because it sees the \ as a make line-continuation character. In particular, the full bash command that my Make invokes is:

python3 -m hailtop.aiotools.copy -vvv 'null' '[             {"from":"src/test/resources","to":"gs://hail-test-ezlis/dking/hail-test-resources/test/resources/"},             {"from":"python/hail/docs/data","to":"gs://hail-test-ezlis/dking/hail-test-resources/doctest/data/"}        ]'

danking · 2024-01-12T18:41:27Z

e.g.

# cat Makefile
foo:
	echo '[ \
hello \
'
# make foo
echo '[ \
hello \
'
[ hello

If you create such a Makefile and run Make do you see [ hello echo'd or do you see something else, possibly including newlines?

danking · 2024-01-12T18:44:55Z

Good catch on the dependency issue, that should be fixed now.

ehigham · 2024-01-12T22:02:25Z

$ make --version    
GNU Make 4.4.1
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2023 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

ehigham · 2024-01-12T22:39:47Z

According to the makefile documentation https://www.gnu.org/software/make/manual/html_node/Splitting-Recipe-Lines.html:

Notice how the backslash/newline pair was removed inside the string quoted with double quotes ("…"), 
but not from the string quoted with single quotes ('…'). This is the way the default shell (/bin/sh) 
handles backslash/newline pairs. If you specify a different shell in your makefiles it may treat them differently.

Seems you (or brew) may have configured make to use something other than /bin/sh.
Quick way to verify:

.PHONY: print-shell
print-shell:
	@echo $(SHELL)

ehigham · 2024-01-12T22:40:20Z

hail/Makefile

+	python3 -m hailtop.aiotools.copy -vvv 'null' '[\
+             {"from":"src/test/resources","to":"$(CLOUD_HAIL_TEST_RESOURCES_DIR)"},\
+             {"from":"python/hail/docs/data","to":"$(CLOUD_HAIL_DOCTEST_DATA_DIR)"}\
+        ]' --timeout 600


This way would seem to be most compatible across platforms:

Suggested change

python3 -m hailtop.aiotools.copy -vvv 'null' '[\

{"from":"src/test/resources","to":"$(CLOUD_HAIL_TEST_RESOURCES_DIR)"},\

{"from":"python/hail/docs/data","to":"$(CLOUD_HAIL_DOCTEST_DATA_DIR)"}\

]' --timeout 600

python3 -m hailtop.aiotools.copy -vvv 'null' "[\

{\"from\":\"src/test/resources\",\"to\":\"$(CLOUD_HAIL_TEST_RESOURCES_DIR)\"},\

{\"from\":\"python/hail/docs/data\",\"to\":\"$(CLOUD_HAIL_DOCTEST_DATA_DIR)\"}\

]" --timeout 600

(This or define SHELL in all makefiles to make them portable)

danking · 2024-01-16T19:51:54Z

EDIT2:

OK, so, I'm not sure when this behavior changed but Make 4.0 wants a \ to indicate that the recipe continues on the next line but also passes that backslash and newline to the shell. In Make 3.81, the \ was also required but the newline and backslash are not passed to the shell. In other words: in 3.81, backslash-newline is always replaced with a space and in 4.0, backslash-newline is replaced with a space except on recipe lines in which case it is necessary to indicate the recipe continues but it is also passed literally to the shell.

The docs page you linked directly addresses our use case and suggests we put the command inside of a make variable (thus triggering normal backslash-newline rules rather than the special recipe line rules).

Sometimes you want to split a long line inside of single quotes, but you don’t want the backslash/newline to appear in the quoted content. This is often the case when passing scripts to languages such as Perl, where extraneous backslashes inside the script can change its meaning or even be a syntax error. One simple way of handling this is to place the quoted string, or even the entire command, into a make variable then use the variable in the recipe. In this situation the newline quoting rules for makefiles will be used, and the backslash/newline will be removed. If we rewrite our example above using this method:
HELLO = 'hello \
world'

all : ; @echo $(HELLO)
we will get output like this:
hello world
If you like, you can also use target-specific variables (see Target-specific Variable Values) to obtain a tighter correspondence between the variable and the recipe that uses it.

It seems to me like there are not any great choices. Putting the JSON into a Make variable seems too magical and likely to confuse a newbie editing this file. Using escaped double quotes is less legible than literal JSON. Putting the whole JSON array on one line is quite long.

I guess we can go with double quotes for now. I tested on Make 3.81 and Make 4.4.1

The first EDIT and the original comment follow for context.

EDIT: Nope, I still appear to be wrong. Hold on.

I have bash 3.2.57

(base) dking@wm28c-761 /tmp % make print-shell
/bin/sh
(base) dking@wm28c-761 /tmp % /bin/sh --version
GNU bash, version 3.2.57(1)-release (arm64-apple-darwin22)
Copyright (C) 2007 Free Software Foundation, Inc.

Looks like this was an intentionally backwards incompatible change in Make 4.0 which removed the POSIX-compatible behavior on which our Makefile relies:

* WARNING: Backward-incompatibility!
  If .POSIX is specified, then make adheres to the POSIX backslash/newline
  handling requirements, which introduces the following changes to the
  standard backslash/newline handling in non-recipe lines:
  * Any trailing space before the backslash is preserved
  * Each backslash/newline (plus subsequent whitespace) is converted to a
    single space

They seem to have broken the behavior in order to fix something else and then added a .POSIX escape hatch for Makefiles that want POSIX compatibility.

For the Makefile I shared above, I get equivalent behavior with and without a .POSIX: fake target. I suspect Make 4.x treats them differently.

In particular, the problem is not the shell, it's our Makes behaving differently. Make 3 replaces \\\n with before calling the shell whereas Make 4 appears to preserve the \\\n to the shell (regardless of the presence of quotes in either case). In particular, Make 4 appears to treat recipe lines differently from how it treats all other lines in Makefiles.

I don't like the noise of the backslash-quotes. The end of the doc page you linked suggests using a Make variable when you intentionally want normal Makefile backslash-newline interpretation. That feels a bit too clever. Let's try just demanding POSIX-compatibility. I've pushed that change. Can you give it a try?

ehigham · 2024-01-16T22:50:50Z

You're absolutely right - my /bin/sh is also a symlink to bash!

ehigham

Works like a charm

danking assigned ehigham Jan 11, 2024

danking mentioned this pull request Jan 11, 2024

[fs] basic sync tool #13834

Closed

fix azure and aws handling of timeout

256a5cc

cleanup dependencies, ensure hailtop is installed

ca41242

timeout is not required

3d2445d

ehigham reviewed Jan 12, 2024

View reviewed changes

danking closed this Jan 16, 2024

danking reopened this Jan 16, 2024

Dan King added 2 commits January 16, 2024 14:52

demand POSIX compatibility.

c440355

actually fix the JSON escaping

630ffd3

ehigham approved these changes Jan 16, 2024

View reviewed changes

danking merged commit d2a8760 into hail-is:main Jan 16, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[httpx][Makefile][copier] add --timeout to copier; use in Makefile #14138

[httpx][Makefile][copier] add --timeout to copier; use in Makefile #14138

danking commented Jan 11, 2024 •

edited

Loading

danking commented Jan 11, 2024

ehigham commented Jan 11, 2024

ehigham commented Jan 11, 2024

danking commented Jan 12, 2024

danking commented Jan 12, 2024 •

edited

Loading

danking commented Jan 12, 2024

danking commented Jan 12, 2024

ehigham commented Jan 12, 2024 •

edited

Loading

ehigham commented Jan 12, 2024 •

edited

Loading

ehigham Jan 12, 2024 •

edited

Loading

danking commented Jan 16, 2024 •

edited

Loading

ehigham commented Jan 16, 2024

ehigham left a comment

[httpx][Makefile][copier] add --timeout to copier; use in Makefile #14138

[httpx][Makefile][copier] add --timeout to copier; use in Makefile #14138

Conversation

danking commented Jan 11, 2024 • edited Loading

danking commented Jan 11, 2024

ehigham commented Jan 11, 2024

ehigham commented Jan 11, 2024

danking commented Jan 12, 2024

danking commented Jan 12, 2024 • edited Loading

danking commented Jan 12, 2024

danking commented Jan 12, 2024

ehigham commented Jan 12, 2024 • edited Loading

ehigham commented Jan 12, 2024 • edited Loading

ehigham Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

danking commented Jan 16, 2024 • edited Loading

ehigham commented Jan 16, 2024

ehigham left a comment

Choose a reason for hiding this comment

danking commented Jan 11, 2024 •

edited

Loading

danking commented Jan 12, 2024 •

edited

Loading

ehigham commented Jan 12, 2024 •

edited

Loading

ehigham commented Jan 12, 2024 •

edited

Loading

ehigham Jan 12, 2024 •

edited

Loading

danking commented Jan 16, 2024 •

edited

Loading