New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Flush stdout and stderr on each write when debugging #843

Merged
merged 3 commits into from Dec 21, 2018

Conversation

Projects
None yet
4 participants
@ndobryanskyy
Copy link
Contributor

ndobryanskyy commented Dec 7, 2018

Issue #, if available:
#835
Description of changes:

Introduced StreamWriter which wraps stdout and stderr and is passed to the Lambda container. This class supports auto_flushing and could be further extended to support any flexibility in output as we want.

Checklist:

  • Write unit tests
  • Write/update functional tests. Do I need any?
  • Write/update integration tests. Do I need any?
  • make pr passes
  • Write documentation. Updated all existing doc strings referring to stdout and stderr

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

@@ -256,6 +259,10 @@ def parameter_overrides(self):

return self._parameter_overrides

@property
def _is_debugging(self):
return bool(self._debug_context)

This comment has been minimized.

@TheSriram

TheSriram Dec 7, 2018

Contributor

Nice, we evaluate on the entire debug context, instead of just the port, like we used to do.

34c7fb0

@@ -0,0 +1,37 @@
"""
This class acts like a wrapper around output streams to provide any flexibility with output we need
"""

This comment has been minimized.

@TheSriram

TheSriram Dec 7, 2018

Contributor

I'm curious to know what is the performance hit, if we just flush all the time.

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Dec 7, 2018

Contributor

I think it is completely negligible for the debugging, thoughts?

This comment has been minimized.

@TheSriram

TheSriram Dec 7, 2018

Contributor

yeah totally we need it for debugging. I'm talking about non-debugging cases as well. just curious.

This comment has been minimized.

@ndobryanskyy

ndobryanskyy Dec 7, 2018

Contributor

I don't know, to be honest, so it needs some investigation

@gyfis

This comment has been minimized.

Copy link

gyfis commented Dec 7, 2018

While I don't have much context about the overall direction of the sam cli, I really like these changes and the fact that it's not just an if-clause somewhere, but a more complete approach to the problem. Thank you for taking the time to work on this :)

@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Dec 7, 2018

@TheSriram I wonder though why on python3.7 one integration test fail?

@jfuss

This comment has been minimized.

Copy link
Contributor

jfuss commented Dec 7, 2018

The Golang zip runtime test is being funky on 3.6 and 3.7 as of recently. I just restarted it. We aren't entirely sure why it fails sometimes at the moment.

@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Dec 7, 2018

@jfuss, understood, pretty strange 😅. Thanks for restarting tho, now it passed just fine :D

@TheSriram
Copy link
Contributor

TheSriram left a comment

LGTM

@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Dec 11, 2018

@TheSriram, thanks for the approve, the only thing that concerns me is that should we really think about "flushing all the time" as it is actually requested in #835. But it seems to me as a story for another PR, tho.

Because my PR actually fixes only cases intended for debugging, thoughts?

@TheSriram

This comment has been minimized.

Copy link
Contributor

TheSriram commented Dec 11, 2018

Correct, that is how I'm thinking of this as well. This PR adds mechanisms around how we flush. But flushing all the time should definitely be a separate PR, which validates performance and what are the impacts of such an operation.

@ndobryanskyy

This comment has been minimized.

Copy link
Contributor

ndobryanskyy commented Dec 11, 2018

Definitely, also I'm looking forward to investigate unbuffered streams in Python3 (in Python2 they just work), that should be a good thing (performancewise) if we want "auto flush" all the time behavior

@TheSriram TheSriram merged commit c849e3c into awslabs:develop Dec 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment