fix: allow for stdout and stderr streams to be unbufferred directly. #708

sriram-mv · 2018-10-10T22:57:55Z

Issue #, if available:

N/A ( I can create one and reference it)

Description of changes:

Allow for stdout and stderr streams to be unbuffered by default.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sanathkr

What are the performance impacts of making the streams unbuffered? How does this work/degrade in Windows cmd prompt and other non-iterm like terminals? How about through a SSH terminal?

I'd like us to be a bit thorough before enabling it globally.. I am okay if this is selectively added only when debugging because I know we might block the process for too long on sockets at that point..

sanathkr · 2018-10-11T00:18:39Z

samcli/cli/main.py

 @click.command(cls=BaseCommand)
+@unbuffered_streams


can we do this selectively only for debugging?

For all of debugging? Can we scope this further down than that?

we can scope it down to say "if you specify a debug port", I think its a fair trade off for performance.

sriram-mv · 2018-10-11T00:23:38Z

Yeah we can add it to debugging mode. That works, in terms of comparison, can we run the integration tests with a timer with and without this change on every platform and verify performance impact? Will that work?

…

On Wed, Oct 10, 2018 at 5:20 PM, Sanath Kumar Ramesh < ***@***.***> wrote: ***@***.**** requested changes on this pull request. What are the performance impacts of making the streams unbuffered? How does this work/degrade in Windows cmd prompt and other non-iterm like terminals? How about through a SSH terminal? I'd like us to be a bit thorough before enabling it globally.. I am okay if this is selectively added only when debugging because I know we might block the process for too long on sockets at that point.. ------------------------------ In samcli/cli/main.py <#708 (comment)>: > @click.command(cls=BaseCommand) ***@***.***_streams can we do this selectively only for debugging? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#708 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADmJlnZDV4V1ersQrQ5woi1OfKbKC4Qtks5ujo7dgaJpZM4XWe1o> .

sriram-mv · 2018-10-11T23:23:54Z

So I ran the integration tests with and without this commit on the local invoke path, and found no difference in timings.

Without this commit.

(aws_sam-sN63_SKq) srirammv@localrainbow:16:20:10:~/aws_sam_cli_WS/aws-sam-cli$time pytest tests/integration/local/invoke/
=============================================================================================================== test session starts ================================================================================================================
platform darwin -- Python 3.6.5, pytest-3.0.7, py-1.4.33, pluggy-0.4.0
rootdir: /Users/srirammv/aws_sam_cli_WS/aws-sam-cli, inifile: pytest.ini
plugins: cov-2.4.0
collected 14 items

tests/integration/local/invoke/test_integrations_cli.py ............
tests/integration/local/invoke/runtimes/test_with_runtime_zips.py ..

============================================================================================================ 14 passed in 55.47 seconds ============================================================================================================

real	0m56.009s
user	0m12.071s
sys	0m2.177s

With commit

(aws_sam-sN63_SKq) srirammv@localrainbow:16:18:38:~/aws_sam_cli_WS/aws-sam-cli$time pytest tests/integration/local/invoke/
=============================================================================================================== test session starts ================================================================================================================
platform darwin -- Python 3.6.5, pytest-3.0.7, py-1.4.33, pluggy-0.4.0
rootdir: /Users/srirammv/aws_sam_cli_WS/aws-sam-cli, inifile: pytest.ini
plugins: cov-2.4.0
collected 14 items

tests/integration/local/invoke/test_integrations_cli.py ............
tests/integration/local/invoke/runtimes/test_with_runtime_zips.py ..

============================================================================================================ 14 passed in 53.78 seconds ============================================================================================================

real	0m54.262s
user	0m11.396s
sys	0m2.190s

Also setting os.environ will only matter for that session of the invoke.

sanathkr

Makes more sense now. Thanks!

sanathkr · 2018-10-16T01:59:50Z

samcli/commands/local/lib/debug_context.py

@@ -13,6 +14,7 @@ def __init__(self,
        self.debug_port = debug_port
        self.debugger_path = debugger_path
        self.debug_args = debug_args
+        os.environ["PYTHONUNBUFFERED"] = "1" if self.debug_port else "0"


Actually on second thought, can you skip the setting of "0" to facilitate usage of this class in multi-threaded scenarios. If you do

if self.debug_port: os.environ[...] = 1

then you don't have the race condition to worry about.

sanathkr

Thanks!

* develop: chore: Version bump aws-sam-translator to 1.8.0 (aws#732) docs: add instructions for using local version of SAM Transformer (aws#688) docs: Update Docker command to docker (aws#725) fix: Iterate over query param list chore: Enable Travis to run integ tests chore(0.6.1): SAM CLI Version bump (aws#717) fix(init/readme): Correct permissions for AWS CLI under requirements (aws#713) add pytest-mock for testing (aws#703) fix: allow for stdout and stderr streams to be unbufferred directly. (aws#708) docs: Add installation instructions for linux (Centos) (aws#670) fix: Updated isBase64Encoded value to bool (aws#699) fix: correct launch.json for nodejs debugging through VSCode (aws#704) docs(usage): Update how to debug Python functions using VS Code (aws#694) docs(Cloud9): Reset bash cache on Cloud9 (aws#693) docs: Updated virtualenv alias name for 3.7 in guide. (aws#706) chore: update aws-sam-translator to 1.7.0 (aws#682) feat: travis CI support for Python 3.7 (aws#679) docs: Update generate-sample-event-payloads link (aws#702) Fix: Raise error for invalid environment variables file (aws#675)

fix: allow for stdout and stderr streams to be unbufferred directly.

4aa82a8

sriram-mv requested review from sanathkr and jfuss October 10, 2018 22:58

sanathkr suggested changes Oct 11, 2018

View reviewed changes

sanathkr approved these changes Oct 16, 2018

View reviewed changes

sanathkr suggested changes Oct 16, 2018

View reviewed changes

fix: only set unbuffered streams when a debug port is set

ec64286

sriram-mv force-pushed the python_unbufferred branch from f35b8df to ec64286 Compare October 16, 2018 17:01

sanathkr suggested changes Oct 16, 2018

View reviewed changes

Merge branch 'develop' into python_unbufferred

a0233fb

sanathkr approved these changes Oct 16, 2018

View reviewed changes

sriram-mv merged commit 34c7fb0 into aws:develop Oct 16, 2018

sriram-mv mentioned this pull request Oct 19, 2018

Release: 0.6.1 Release #720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow for stdout and stderr streams to be unbufferred directly. #708

fix: allow for stdout and stderr streams to be unbufferred directly. #708

sriram-mv commented Oct 10, 2018

sanathkr left a comment

sanathkr Oct 11, 2018

jfuss Oct 11, 2018

sriram-mv Oct 11, 2018

sriram-mv commented Oct 11, 2018 via email

sriram-mv commented Oct 11, 2018

sanathkr left a comment

sanathkr Oct 16, 2018

sanathkr left a comment

		@click.command(cls=BaseCommand)
		@unbuffered_streams

fix: allow for stdout and stderr streams to be unbufferred directly. #708

fix: allow for stdout and stderr streams to be unbufferred directly. #708

Conversation

sriram-mv commented Oct 10, 2018

sanathkr left a comment

Choose a reason for hiding this comment

sanathkr Oct 11, 2018

Choose a reason for hiding this comment

jfuss Oct 11, 2018

Choose a reason for hiding this comment

sriram-mv Oct 11, 2018

Choose a reason for hiding this comment

sriram-mv commented Oct 11, 2018 via email

sriram-mv commented Oct 11, 2018

sanathkr left a comment

Choose a reason for hiding this comment

sanathkr Oct 16, 2018

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment