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

BF: check for stdin/out/err to not be closed before checking for .isatty #3268

Merged
merged 3 commits into from Apr 3, 2019

Conversation

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Mar 24, 2019

Closes #3267

TODOs:

  • test
  • try to figure out what change in our behavior lead to the benchmark to start failing after Aug 8 2018. See #3267
@codecov
Copy link

@codecov codecov bot commented Mar 24, 2019

Codecov Report

Merging #3268 into 0.11.x will decrease coverage by 11.45%.
The diff coverage is 55.55%.

Impacted file tree graph

@@             Coverage Diff             @@
##           0.11.x    #3268       +/-   ##
===========================================
- Coverage    90.8%   79.34%   -11.46%     
===========================================
  Files         252      263       +11     
  Lines       33016    40803     +7787     
===========================================
+ Hits        29980    32375     +2395     
- Misses       3036     8428     +5392
Impacted Files Coverage Δ
datalad/tests/test_utils.py 88.27% <100%> (-8.12%) ⬇️
datalad/utils.py 73.52% <50%> (-14.01%) ⬇️
datalad/interface/tests/test_annotate_paths.py 60.96% <0%> (-39.04%) ⬇️
datalad/interface/tests/test_run_procedure.py 63.12% <0%> (-36.88%) ⬇️
datalad/tests/test_testrepos.py 63.15% <0%> (-36.85%) ⬇️
datalad/distribution/tests/test_subdataset.py 65.35% <0%> (-34.65%) ⬇️
datalad/plugin/tests/test_addurls.py 66.24% <0%> (-33.76%) ⬇️
datalad/interface/tests/test_run.py 66.92% <0%> (-32.93%) ⬇️
datalad/interface/run.py 67.3% <0%> (-32.7%) ⬇️
datalad/distribution/tests/test_update.py 68.02% <0%> (-31.98%) ⬇️
... and 65 more

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 8b61e85...b5e0edb. Read the comment docs.

datalad/utils.py Outdated
return (
(not sys.stdin.closed and sys.stdin.isatty()) and
(not sys.stdout.closed and sys.stdout.isatty()) and
(not sys.stderr.closed and sys.stderr.isatty())
Copy link
Member Author

@yarikoptic yarikoptic Apr 2, 2019

oh hoh, .closed itself pukes if stdout is closed

$> python2 /tmp/1.py 
Traceback (most recent call last):
  File "/tmp/1.py", line 3, in <module>
    print(sys.stdout.closed)
ValueError: I/O operation on closed file

$> python3 /tmp/1.py 
Traceback (most recent call last):
  File "/tmp/1.py", line 3, in <module>
    print(sys.stdout.closed)
ValueError: I/O operation on closed file.

$> cat /tmp/1.py    
import sys; 
sys.stdout.close(); 
print(sys.stdout.closed)

so I think there is no way around try/except :-/

Copy link
Contributor

@kyleam kyleam left a comment

I'm not sure that is_interactive is the spot to guard against a closed stream. If we conflate closed streams with the normal isatty() -> false, won't downstream code just fail when trying to work with the closed streams? By catching the exception (or otherwise swallowing it) in is_interactive, aren't we just delaying the issue?

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Apr 2, 2019

legit concern, but

  • we do not close streams, so I do not think we should guard downstream "too much". If anything relevant is closed, I expect code to crash at the appropriate point.
  • how probable is the situation? ATM we have only a single use-case - running benchmarks under asv. Note that no other errors emerge AFAIK -- just for is_interactive, which gets fixed here (and there would be follow up PR as well to monkey patch benchmarks setup)

PS just mentioned that I managed to submit it against master, although I think it is well worth 0.11.x. will rebase

@yarikoptic yarikoptic changed the base branch from master to 0.11.x Apr 2, 2019
@kyleam
Copy link
Contributor

@kyleam kyleam commented Apr 2, 2019

  • we do not close streams, so I do not think we should guard downstream "too much". If anything relevant is closed, I expect code to crash at the appropriate point.

Agreed, my suggestion wasn't that we guard downstream. I think we should assume that the streams are open and let an exception be raised---including in is_interactive(). This patch is making is_interactive report "false" when the answer is really "neither/invalid question" (which the exception describes nicely).

  • how probable is the situation? ATM we have only a single use-case - running benchmarks under asv. Note that no other errors emerge AFAIK -- just for is_interactive, which gets fixed here (and there would be follow up PR as well to monkey patch benchmarks setup)

I'd think not likely at all. Again, I'd say we should assume that std{in,out,err} are open. Which makes me think that this should be fixed upstream (why does asv result in stdin being closed?)---or at least make a more local, well-documented kludge outside of is_interactive().

But I won't push back more than that because I don't think it matters all that much in practice.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Apr 2, 2019

Which makes me think that this should be fixed upstream (why does asv result in stdin being closed?)---or at least make a more local, well-documented kludge outside of is_interactive().

yeah, I was on a fence to either do the archaeological study on when it was introduced to asv... my argumentation in support of "it is ok" was that I think it should not be guaranteed that stdin is open. My wild guess is that it was intentional on their end to guarantee that no input should somehow be expected. That is why I also think thought it is ok for is_interactive to just return False in such a case. Hence

This patch is making is_interactive report "false" when the answer is really "neither/invalid question" (which the exception describes nicely).

could be argued or clarified in documentation - IMHO the main purpose is to detect when environment is interactive and thus all streams are open and tty. If something is not satisfying that -- it is just False. I do not see any advantage for is_interactive throwing an exception when environment clearly is not interactive.
While at it I even wondered if we shouldn't check if stderr is a tty. I think it should be ok to redirect stderr to a file while stdin/out interacting with the user... but I guess it is a separate issue/PR

Overall:

  • let's check with upstream: airspeed-velocity/asv#802
  • I will push a slight tune up to is_interactive docstring describing its "resilience".

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Apr 3, 2019

appveyor fail - unrelated #3287
travis -- probably due to my data transfer from the server its connection was more flaky. restarted those

@kyleam
Copy link
Contributor

@kyleam kyleam commented Apr 3, 2019

appveyor fail - unrelated #3287

The error from 3287 doesn't appear in the failure here. The only failure I spot in that run is

Invoke-WebRequest : 503 Service Temporarily Unavailable
nginx/1.13.12
[...]

Looks unrelated in any case.

@kyleam kyleam merged commit df03894 into datalad:0.11.x Apr 3, 2019
2 of 3 checks passed
yarikoptic added a commit that referenced this issue Apr 6, 2019
    ## 0.11.4 (Mar 18, 2019) -- get-ready

    Largely a bug fix release with a few enhancements

    ### Important

    - 0.11.x series will be the last one with support for direct mode of [git-annex][]
      which is used on crippled (no symlinks and no locking) filesystems.
      v7 repositories should be used instead.

    ### Fixes

    - Extraction of .gz files is broken without p7zip installed.  We now
      abort with an informative error in this situation.  ([#3176][])

    - Committing failed in some cases because we didn't ensure that the
      path passed to `git read-tree --index-output=...` resided on the
      same filesystem as the repository.  ([#3181][])

    - Some pointless warnings during metadata aggregation have been
      eliminated.  ([#3186][])

    - With Python 3 the LORIS token authenticator did not properly decode
      a response ([#3205][]).

    - With Python 3 downloaders unnecessarily decoded the response when
      getting the status, leading to an encoding error.  ([#3210][])

    - In some cases, our internal command Runner did not adjust the
      environment's `PWD` to match the current working directory specified
      with the `cwd` parameter.  ([#3215][])

    - The specification of the pyliblzma dependency was broken.  ([#3220][])

    - [search] displayed an uninformative blank log message in some
      cases.  ([#3222][])

    - The logic for finding the location of the aggregate metadata DB
      anchored the search path incorrectly, leading to a spurious warning.
      ([#3241][])

    - Some progress bars were still displayed when stdout and stderr were
      not attached to a tty.  ([#3281][])

    - Check for stdin/out/err to not be closed before checking for `.isatty`.
      ([#3268][])

    ### Enhancements and new features

    - Creating a new repository now aborts if any of the files in the
      directory are tracked by a repository in a parent directory.
      ([#3211][])

    - [run] learned to replace the `{tmpdir}` placeholder in commands with
      a temporary directory.  ([#3223][])

    - [duecredit][] support has been added for citing DataLad itself as
      well as datasets that an analysis uses.  ([#3184][])

    - The `eval_results` interface helper unintentionally modified one of
      its arguments.  ([#3249][])

    - A few DataLad constants have been added, changed, or renamed ([#3250][]):
      - `HANDLE_META_DIR` is now `DATALAD_DOTDIR`.  The old name should be
         considered deprecated.
      - `METADATA_DIR` now refers to `DATALAD_DOTDIR/metadata` rather than
        `DATALAD_DOTDIR/meta` (which is still available as
        `OLDMETADATA_DIR`).
      - The new `DATASET_METADATA_FILE` refers to `METADATA_DIR/dataset.json`.
      - The new `DATASET_CONFIG_FILE` refers to `DATALAD_DOTDIR/config`.
      - `METADATA_FILENAME` has been renamed to `OLDMETADATA_FILENAME`.

* tag '0.11.4': (82 commits)
  Updated CHANGELOG.md for having merged check for not being closed
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.4 entries
  CHANGELOG.md: Update 0.11.4 entries
  CHANGELOG.md: Adjust the format of a link
  BF: split lines on spaces and commas befoe doing sed capture of #issue
  RF: adjust tools/link_issues_CHANGELOG to have MD links as [#issue][]
  BF+DOC: Fix all links to mark them valid markdown
  BF: Fix markup bug that prevents sphinx-build from succeeding
  DOC: extend coumentation of is_interactive
  BF: guard .istty with try/except
  ENH: ui: Drop progress bars if not attached to tty
  ENH: ui: Add another name for SilentConsoleLog
  BF: check for stdin/out/err to not be closed before checking for .isatty
  RF: HANDLE_META_DIR -> DATALAD_DOTDIR (but keeping an alias for compatibility)
  RF: define/use consts DATASET_CONFIG_FILE, DATASET_METADATA_FILE, METADATA_DIR
  RF: METADATA_DIR/FILE -> OLDMETADATA_DIR/FILE
  BF: do not cause a side-effect on kwargs in @eval_results
  BF: join with ds.path when trying to see if any other metadata file is available
  RF: Move duecredit into its own section so it is not a part of install_requires
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.4 entries
  ...
yarikoptic added a commit to yarikoptic/datalad that referenced this issue Apr 6, 2019
    ## 0.11.4 (Mar 18, 2019) -- get-ready

    Largely a bug fix release with a few enhancements

    ### Important

    - 0.11.x series will be the last one with support for direct mode of [git-annex][]
      which is used on crippled (no symlinks and no locking) filesystems.
      v7 repositories should be used instead.

    ### Fixes

    - Extraction of .gz files is broken without p7zip installed.  We now
      abort with an informative error in this situation.  ([datalad#3176][])

    - Committing failed in some cases because we didn't ensure that the
      path passed to `git read-tree --index-output=...` resided on the
      same filesystem as the repository.  ([datalad#3181][])

    - Some pointless warnings during metadata aggregation have been
      eliminated.  ([datalad#3186][])

    - With Python 3 the LORIS token authenticator did not properly decode
      a response ([datalad#3205][]).

    - With Python 3 downloaders unnecessarily decoded the response when
      getting the status, leading to an encoding error.  ([datalad#3210][])

    - In some cases, our internal command Runner did not adjust the
      environment's `PWD` to match the current working directory specified
      with the `cwd` parameter.  ([datalad#3215][])

    - The specification of the pyliblzma dependency was broken.  ([datalad#3220][])

    - [search] displayed an uninformative blank log message in some
      cases.  ([datalad#3222][])

    - The logic for finding the location of the aggregate metadata DB
      anchored the search path incorrectly, leading to a spurious warning.
      ([datalad#3241][])

    - Some progress bars were still displayed when stdout and stderr were
      not attached to a tty.  ([datalad#3281][])

    - Check for stdin/out/err to not be closed before checking for `.isatty`.
      ([datalad#3268][])

    ### Enhancements and new features

    - Creating a new repository now aborts if any of the files in the
      directory are tracked by a repository in a parent directory.
      ([datalad#3211][])

    - [run] learned to replace the `{tmpdir}` placeholder in commands with
      a temporary directory.  ([datalad#3223][])

    - [duecredit][] support has been added for citing DataLad itself as
      well as datasets that an analysis uses.  ([datalad#3184][])

    - The `eval_results` interface helper unintentionally modified one of
      its arguments.  ([datalad#3249][])

    - A few DataLad constants have been added, changed, or renamed ([datalad#3250][]):
      - `HANDLE_META_DIR` is now `DATALAD_DOTDIR`.  The old name should be
         considered deprecated.
      - `METADATA_DIR` now refers to `DATALAD_DOTDIR/metadata` rather than
        `DATALAD_DOTDIR/meta` (which is still available as
        `OLDMETADATA_DIR`).
      - The new `DATASET_METADATA_FILE` refers to `METADATA_DIR/dataset.json`.
      - The new `DATASET_CONFIG_FILE` refers to `DATALAD_DOTDIR/config`.
      - `METADATA_FILENAME` has been renamed to `OLDMETADATA_FILENAME`.

* tag '0.11.4':
  Updated CHANGELOG.md for having merged check for not being closed
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.4 entries
  CHANGELOG.md: Update 0.11.4 entries
  CHANGELOG.md: Adjust the format of a link
  BF: split lines on spaces and commas befoe doing sed capture of #issue
  RF: adjust tools/link_issues_CHANGELOG to have MD links as [#issue][]
  BF+DOC: Fix all links to mark them valid markdown
  BF: Fix markup bug that prevents sphinx-build from succeeding
  DOC: extend coumentation of is_interactive
  BF: guard .istty with try/except
  BF: check for stdin/out/err to not be closed before checking for .isatty
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.4 entries
  CHANGELOG: Add entries for 0.11.4
  ENH: version boost and initial changes to CHANGELOG
yarikoptic added a commit to yarikoptic/datalad that referenced this issue Apr 9, 2019
…g benchmarks on older commits

Proper fix within DataLad itself is in datalad#3268 .
This change to benchmarks should monkey patch older datalad "runtime" so it would
not puke on those benchmarks relying on is_interactive
@yarikoptic yarikoptic added this to the Release 0.11.4 milestone Apr 15, 2019
@yarikoptic yarikoptic deleted the bf-stdin-closed branch Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants