这是indexloc提供的服务，不要输入任何密码
Skip to content

Enabling "conda run" to display real time output for the subprocess #10270

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Oct 12, 2020
Merged

Enabling "conda run" to display real time output for the subprocess #10270

merged 6 commits into from
Oct 12, 2020

Conversation

@gabrielcnr
Copy link
Contributor

@gabrielcnr gabrielcnr commented Oct 8, 2020

This enables the command conda run to display the live output from the subprocess's stdout and stderr, in real time.

The current behaviour-- which prints the stdout and stderr only after the process has finished --remains unchanged.

The conda run command now can take a flag --live-stream which would flip the behaviour to provide the real time output.

I believe this is related to: #9412

@gabrielcnr
conda run: Enabling to display real time output for the subprocess
afa1e62 
FIX: inline p.poll() check

Added unit tests for subprocess_call() with live stream

Removed comment + minor changes
@gabrielcnr gabrielcnr requested a review from a team as a code owner October 8, 2020 16:52
@anaconda-issue-bot anaconda-issue-bot added the cla-signed [bot] added once the contributor has signed the CLA label Oct 8, 2020
@marcelotrevisani
Copy link
Member

marcelotrevisani commented Oct 8, 2020

Hi @chenghlee, could you please take a look on this PR?

@chenghlee chenghlee added this to the 4.9.0 milestone Oct 9, 2020
mingwandroid
mingwandroid approved these changes Oct 10, 2020
View reviewed changes
Copy link
Contributor

@mingwandroid mingwandroid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd probably say it should be the default to be honest with you. Also, can you handle stdin here? Seems like it should be possible.

@gabrielcnr
Copy link
Contributor Author

gabrielcnr commented Oct 10, 2020

I think this should be the default too. I didn't make it, because I wasn't sure if would be desirable to break the current behaviour, so I opted for keeping it backwards compatible. But I guess the impact of the change is not big.
As for the stdin, I think we can address that in a separate PR in the future. What do you think @mingwandroid, @marcelotrevisani ?

@marcelotrevisani
Copy link
Member

marcelotrevisani commented Oct 10, 2020

I think this should be the default too. I didn't make it, because I wasn't sure if would be desirable to break the current behaviour, so I opted for keeping it backwards compatible. But I guess the impact of the change is not big.
As for the stdin, I think we can address that in a separate PR in the future. What do you think @mingwandroid, @marcelotrevisani ?

I agree that @mingwandroid suggestion can be addressed after as the modification to also support the stdin will be a bit more complicated to implement. And I also agree with @mingwandroid that it should be the default behaviour.

@gabrielcnr
Copy link
Contributor Author

gabrielcnr commented Oct 12, 2020

Thanks for the feedback @marcelotrevisani and @mingwandroid. As part of this PR, can you please confirm if I should:

  1. make True the default option for the --live-stream flag
    or
  2. remove the --live-stream flag entirely, but keep the live stream behaviour as the new default behaviour
    or
  3. nothing

Please let me know and I will happily make the changes. Thank you.

@marcelotrevisani
Copy link
Member

marcelotrevisani commented Oct 12, 2020

Thanks for the feedback @marcelotrevisani and @mingwandroid. As part of this PR, can you please confirm if I should:

  1. make True the default option for the --live-stream flag
    or
  2. remove the --live-stream flag entirely, but keep the live stream behaviour as the new default behaviour
    or
  3. nothing

Please let me know and I will happily make the changes. Thank you.

I believe that is a question for @chenghlee or @mingwandroid
But anyways we could try to merge it as it is and next release we can let it as a default, but I believe @chenghlee is leading the conda releases so he should know what is the best decision for conda in this case

@marcelotrevisani
Copy link
Member

marcelotrevisani commented Oct 12, 2020

just one thing, could you please resolve the conflicts with your PR?

@chenghlee
Copy link
Contributor

chenghlee commented Oct 12, 2020

Thanks for the feedback @marcelotrevisani and @mingwandroid. As part of this PR, can you please confirm if I should:

  1. make True the default option for the --live-stream flag
    or
  2. remove the --live-stream flag entirely, but keep the live stream behaviour as the new default behaviour
    or
  3. nothing

Please let me know and I will happily make the changes. Thank you.

I believe that is a question for @chenghlee or @mingwandroid
But anyways we could try to merge it as it is and next release we can let it as a default, but I believe @chenghlee is leading the conda releases so he should know what is the best decision for conda in this case

For now (i.e., the 4.9.0 release), let's just leave it as is (i.e., option 3 -- do nothing), since this is a new behavior and I want to avoid any surprises for current users.

I agree that that --live-stream should be the default, but we should roll that change into the 5.0.0 release, where we're planning on making a bunch of other default behavior changes. 5.0.0 is currently scheduled for late 2020-Q4/early 2020-Q1, and that should give us enough time for users to test out this feature and for us to announce upcoming major changes.

@chenghlee
Copy link
Contributor

chenghlee commented Oct 12, 2020

Once I'll unbreak the CI system, I'll review and merge this PR. Expect that to happen sometime later today.

@chenghlee chenghlee merged commit 8cc6f58 into conda:master Oct 12, 2020
@gabrielcnr gabrielcnr deleted the feature/conda-run-live-stream branch October 12, 2020 21:31
@jonashaag
Copy link
Contributor

jonashaag commented Sep 20, 2021

Pardon the naive question but what is the point of --live-stream when we have --no-capture-output?

@jonashaag jonashaag mentioned this pull request Jan 27, 2022
Merged
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Sep 21, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mingwandroid mingwandroid mingwandroid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity

Projects

None yet

Milestone

4.9.0

Development

Successfully merging this pull request may close these issues.

6 participants

@gabrielcnr @marcelotrevisani @chenghlee @jonashaag @mingwandroid @anaconda-issue-bot