Conversation
7c030f8 to
ecf405a
Compare
ecf405a to
1bb6f80
Compare
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
07ed6be to
949d6bc
Compare
63b3b2a to
02b677a
Compare
| f"{Path(*Path(stack()[1].filename).parts[-2:])}:{stack()[1].function}" | ||
| " => " | ||
| f"{Path(*Path(stack()[0].filename).parts[-2:])}:{stack()[0].function}" |
There was a problem hiding this comment.
inspect.stack is a expensive function to call and we should only call it if we really need it. So either:
- we guard this print behind some check, that it will not be executed on each call
- use a better grouping without using the stack
- use debug instead grouping
There was a problem hiding this comment.
debug and guarding are not useful as there is no way to discover we are being run by GitHub Actions with steps debug or not. I think Python has lazy loading so that did not make sense either. It won't hurt anything to call stack only once per invocation, so applied, thanks!
There was a problem hiding this comment.
We currently don't need this code, we are fine to add it if it will only be used when the action is running with debug logs enabled. We don't want to slow down each execution for logs that we currently don't need. Please don't resolve a conversation if you don't agree with the requested changes
There was a problem hiding this comment.
@edenhaus writes:
We currently don't need this code, we are fine to add it if it will only be used when the action is running with debug logs enabled. We don't want to slow down each execution for logs that we currently don't need. Please don't resolve a conversation if you don't agree with the requested changes
(emphasis mine)
Sure, again, how exactly do you want that to be accomplished? I see no existing strategy that would allow checking within python code execution the caller GitHub Actions environment debug state, so are you having something in mind here how to do this ?
There was a problem hiding this comment.
I currently have no time to invest in this topic as our CI is working fine without any issues. We will not merge something that will slow down our CI if we don't need it. Therefore please come up with a proposal, that this debug code is just executed when enabled/needed. I thought about using the debug functionality of GitHub actions as it sounds like a good fit.
There was a problem hiding this comment.
Could we add a --debug flag to the invocation of that python script from github actions, and pass this flag from github actions dependent on the state of secrets (the method of debug invocation from github actions if I am understanding correctly)?
Looks like Pascal, Robert, and Marc are the principal authors there; request for comment if adding a debug flag is appropriate there to signal that state from github actions and/or commandline invocation, and also share some guidance how to gate that import so it does not incur a penalty otherwise?
There was a problem hiding this comment.
looking for response about whether a debug flag added to the python script is acceptable
0b3fdc3 to
341b22f
Compare
341b22f to
e427f2b
Compare
|
rebased to master |
|
Drafting as requested changes are not implemented |
0ba3030 to
f7e8577
Compare
f7e8577 to
34f7f8a
Compare
Let's expose inspect information about the caller and callee filename and function name in the CI/CD output for wheels builder python runtime, and the non-static arguments presented to subprocess.run() * output is grouped instead of debug-only as a balance between verbosity and being noisy * output gates printing env if not empty or None due to concerns about leaking secrets via env * static arguments to subprocess.run() are not presented Co-Authored-By: Robert Resch <robert@resch.dev>
34f7f8a to
ca883ec
Compare
3 participants
builder: utils.py print inspect information for run_command() as group
Let's expose inspect information about the caller and callee filename and
function name in the CI/CD output for wheels builder python runtime, and
the non-static arguments presented to subprocess.run()
output is grouped instead of debug-only as a balance between verbosity
and being noisy
output gates printing env if not empty or None due to concerns about
leaking secrets via env
static arguments to subprocess.run() are not presented
Resolves: #1036
Co-Authored-By: Robert Resch robert@resch.dev