Skip to content
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

Redirect io #69

Merged
merged 11 commits into from
Feb 21, 2022
Merged

Redirect io #69

merged 11 commits into from
Feb 21, 2022

Conversation

akleb
Copy link
Contributor

@akleb akleb commented Oct 7, 2021

Purpose

This PR moves the multipoint redirectIO function to baseclasses. It also includes a new function that allows the user to use a with block that only redirects the IO during the with block. Both of these functions also have increased capability to split the stderr and stdout into different files if desired.

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

I added tests to the new test_redirectIO.py file. I think these are failing because testflo is capturing output and messing with the file streams before the tests are run. I am not sure how we can get around. I have tested this with my own scripts locally and it seems to work.

Checklist

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@akleb akleb requested a review from a team as a code owner October 7, 2021 15:51
@akleb akleb requested review from eirikurj and sseraj October 7, 2021 15:51
@akleb
Copy link
Contributor Author

akleb commented Oct 7, 2021

@A-Gray-94 This begins to address some of what you were looking for in #57

@sseraj sseraj mentioned this pull request Oct 7, 2021
12 tasks
@akleb
Copy link
Contributor Author

akleb commented Oct 7, 2021

I think the problem with testflo has to do with it messing with stderr. Even when you use the -s option testflo still messes with the file descriptors of stderr, making it impossible to call fileno. I do not think that sys.stderr.fileno() should cause an error.

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #69 (9fdd6f3) into master (97e06f3) will decrease coverage by 0.42%.
The diff coverage is 2.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   28.21%   27.79%   -0.43%     
==========================================
  Files          25       26       +1     
  Lines        2307     2342      +35     
==========================================
  Hits          651      651              
- Misses       1656     1691      +35     
Impacted Files Coverage Δ
baseclasses/utils/redirectIO.py 0.00% <0.00%> (ø)
baseclasses/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97e06f3...9fdd6f3. Read the comment docs.

@akleb
Copy link
Contributor Author

akleb commented Oct 12, 2021

I have been using this with both pyHyp and ADflow and they both are redirecting IO properly in a with block, something like this:

f_stream = open(os.path.join(os.path.dirname(output), self.getOption("meshLog")), "w")
with redirectIO.redirectingIO(f_stream):
    hyp = pyHyp(options=options)
    hyp.run()
    hyp.writeCGNS(output + "_L0.cgns")
print("Saying something back in terminal instead of f_stream")

The redirectingIO function closes the f_stream after it is finished with it

@marcomangano
Copy link
Contributor

I have been using this with both pyHyp and ADflow and they both are redirecting IO properly in a with block, something like this:

f_stream = open(os.path.join(os.path.dirname(output), self.getOption("meshLog")), "w")
with redirectIO.redirectingIO(f_stream):
    hyp = pyHyp(options=options)
    hyp.run()
    hyp.writeCGNS(output + "_L0.cgns")
print("Saying something back in terminal instead of f_stream")

The redirectingIO function closes the f_stream after it is finished with it

I think it could be beneficial to add something like this as an example in the function docstrings, I did not fully realize this was the way to call the function

@akleb
Copy link
Contributor Author

akleb commented Oct 13, 2021

Not sure how to get the redirectIO functions in the docs without importing them in baseclasses.utils.__init__.py, which I thought we were moving away from. Or do we just not want to import them in baseclasses.__init__.py?

Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

We should bump the version here as well

@sseraj sseraj requested review from sseraj and removed request for sseraj February 21, 2022 19:15
Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Some minor comments

baseclasses/utils/redirectIO.py Outdated Show resolved Hide resolved
baseclasses/utils/redirectIO.py Outdated Show resolved Hide resolved
baseclasses/utils/redirectIO.py Outdated Show resolved Hide resolved
baseclasses/utils/redirectIO.py Outdated Show resolved Hide resolved
@marcomangano marcomangano merged commit 051af87 into mdolab:master Feb 21, 2022
@akleb akleb deleted the redirectIO branch February 21, 2022 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants