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

dvc pipeline show with ascii flag is broken on ITerm2 #2420

Closed
drorata opened this issue Aug 20, 2019 · 30 comments
Closed

dvc pipeline show with ascii flag is broken on ITerm2 #2420

drorata opened this issue Aug 20, 2019 · 30 comments
Assignees
Labels
bug Did we break something? p2-medium Medium priority, should be done, but less important research

Comments

@drorata
Copy link

drorata commented Aug 20, 2019

Executing dvc pipeline show --ascii <your-dvc-file.dvc> yields an error (which can be found here). Afterward, the terminal is broken and all outputs are TABed in a wired way; the terminal has to be killed.

Replacing --ascii with --dot solves the problem. Otherwise, as per @MrOutis suggestion:

dvc pipeline show --ascii <your-dvc-file.dvc> | less

is a workaround.

@ghost ghost added the bug Did we break something? label Aug 20, 2019
@ghost
Copy link

ghost commented Aug 20, 2019

@ghost
Copy link

ghost commented Aug 20, 2019

Terminal used was iTerm2.

The first step would be to reproduce the error: cbreak() returned ERR

@drorata
Copy link
Author

drorata commented Aug 20, 2019

And TERM=xterm-256color is the result of env | grep term

@ghost
Copy link

ghost commented Aug 20, 2019

I'm not sure if this is something DVC should handle, as it is working in different environments.
We can choose not to use curses and go with less or another file viewer (more), or just output it to STDOUT and let the user pipe it if it is too long to read.

What do you think @iterative/engineering ?

@efiop efiop changed the title dvc pipeline show with ascii flag is broken dvc pipeline show with ascii flag is broken on ITerm2 Aug 20, 2019
@efiop
Copy link
Contributor

efiop commented Aug 20, 2019

@shcheklein
Copy link
Member

My 2 cents - simplify and go with less or something similar. Not sure why should we bring curses, and the whole interface is confusing, not very reliable, etc. The only question - is it possible to do it with less at all, the same experience? Especially when pipeline is pretty wide?

@drorata
Copy link
Author

drorata commented Aug 21, 2019

  1. I rolled back dvc to version 0.51.2 and the "plot" is not broken. Switching back to version 0.54.1 introduces the broken behavior again. Therefore, I would say that's a proof the issue is DVC related.
  2. Along DOTADIW approach, I would second @shcheklein and say that dvc should not be bothered with the rendering of the graph. To that end, yielding a dot representation is the best solution. The user can then decide how to render it.

@drorata
Copy link
Author

drorata commented Aug 21, 2019

@shcheklein BTW, using -S together with less can solve the problem of long lines. Right?

@efiop
Copy link
Contributor

efiop commented Aug 21, 2019

@shcheklein @drorata I disagree with you, guys. We've been asked numerous times to introduce a UI for showing DAG and the only non-distracting way to do it is to print it or to render it in the terminal. Printing would be broken for terminals that don't support horizontal scrolling(e.g. my default ubuntu terminal). Rendering in curses is only temporarily broken for that specific iterm2 terminal in a recent dvc version, for reasons that we will figure out and fix. Other than that, rendering allows us to present pretty much any DAG size on any terminal.

Dot option was introduced very early, but the problem with it is that you need additional actions to render it and then you would only be able to view it in a pop-up GUI window, which is distracting. But even that relies on graphviz, which doesn't have wheels for windows(pygraphviz more precisely), making it a headache to deploy(we are not able to include it as a dependency for windows) and setup. You could use a CLI perl tool for rendering it in the terminal, but that is also not very friendly. If you feel like going with --dot, it is already there and ready to be used 🙂

About the less tool, there is no such tool on windows, to my knowledge. And even if there was, making user pipe it manually is also not very friendly.

@efiop
Copy link
Contributor

efiop commented Aug 21, 2019

@drorata Could you show pip freeze on both versions, please? Might be that some dependency has updated and caused that issue. Or maybe you've had some other package that has downgraded something that broke it in dvc.

@drorata
Copy link
Author

drorata commented Aug 21, 2019

The virtual environment I'm using is based on an environment.yml of conda. So I'm not sure pip freeze will return what you want; maybe it will because dvc is installed using pip. For the record, I rolled back dvc by simply changing the version in the pip section of the corresponding environment.yml --- so the chances that some other dependencies are small.

If you still think this feedback is worthy and needed, let me know and I will be able to check it later.

@efiop
Copy link
Contributor

efiop commented Sep 24, 2019

For the record: another user experiencing the same problem https://discordapp.com/channels/485586884165107732/485596304961962003/626104776202911765

@efiop efiop added the p0-critical Critical issue. Needs to be fixed ASAP. label Sep 24, 2019
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Sep 24, 2019

Agree with previous comments suggesting to remove nice output since it's not universally compatible with all terminals. Users can easily pipe the ascii output to less or redirect to a file 🙂

Unless there's a good reason we're not seeing? A true benefit vs less for example.

@efiop
Copy link
Contributor

efiop commented Sep 24, 2019

For the record: less is overflowing lines that are too long to fit into the terminal, which is fine for text and not acceptable for ascii images. Our solution deals with that.

@drorata
Copy link
Author

drorata commented Sep 25, 2019

@efiop the -S of less handles the overflow as far as I've tested.

@efiop
Copy link
Contributor

efiop commented Sep 25, 2019

@drorata Indeed, didn't know about that one. 🙂

@efiop efiop added p1-important Important, aka current backlog of things to do and removed p0-critical Critical issue. Needs to be fixed ASAP. labels Oct 8, 2019
@efiop efiop self-assigned this Oct 8, 2019
@efiop efiop added the research label Oct 8, 2019
@efiop
Copy link
Contributor

efiop commented Oct 10, 2019

@drorata I'm still not able to reproduce. Could you show printenv | grep TERM please? You've shown a result of a similar command before, but it was case sensitive, resulting in a limited output. Also, are you using some custom profiles for iterm2?

@drorata
Copy link
Author

drorata commented Oct 14, 2019

The output of printenv | grep TERM is:

TERM_SESSION_ID=SOME-KEY
LC_TERMINAL_VERSION=3.3.4
ITERM_PROFILE=Default
TERM_PROGRAM_VERSION=3.3.4
TERM_PROGRAM=iTerm.app
LC_TERMINAL=iTerm2
COLORTERM=truecolor
TERM=xterm-256color
ITERM_SESSION_ID=SOME-OTHER-KEY

@efiop
Copy link
Contributor

efiop commented Oct 19, 2019

@drorata Btw, since you've experienced that issue with dvc installed from conda, could you please try installing using brew install iterative/homebrew-dvc/dvc as see if that works or not?

@drorata
Copy link
Author

drorata commented Oct 20, 2019

@efiop I'd prefer to skip this test on my work computer --- it could be a little messy in terms of mixing environments, isn't it?

@efiop
Copy link
Contributor

efiop commented Oct 20, 2019

@drorata True, but you could uninstall it from conda first. Then it shouldn't cause any issues.

@drorata
Copy link
Author

drorata commented Oct 20, 2019

But it means I'd have to install Python using brew, and this can cause conflicts with my conda based environment... 😬

@efiop
Copy link
Contributor

efiop commented Oct 20, 2019

@drorata when you brew install iterative/homebrew-dvc/dvc, it creates a virtualenv and creates a shim that you are calling dvc through, so python is not exposed to the outside.

@efiop
Copy link
Contributor

efiop commented Oct 20, 2019

@drorata though I am finding some mentions of brew and conda colliding, so looks like I'm wrong.

@efiop
Copy link
Contributor

efiop commented Oct 20, 2019

Alright, I've tried with multiple versions of conda, as well us regular python (since the other user was experiencing that without conda) and I'm not able to reproduce. Double checked changed in our reqs, but can't put my finger on anything. Tried with multiple versions of asciimatics too.

Since there are plenty of workarounds, I'm postponing this until there is more info. It would be great if anyone who is able to reproduce could do some research,.

@efiop efiop added p2-medium Medium priority, should be done, but less important and removed p1-important Important, aka current backlog of things to do labels Oct 20, 2019
@efiop
Copy link
Contributor

efiop commented Nov 11, 2019

@drorata Could you try running TERM=linux dvc pipeline show --ascii my.dvc please? We've had something similar reported in #2768 , and that trick worked pretty well. Looks like both are ncurses bugs, though I can't quite put my finger on anything yet.

@drorata
Copy link
Author

drorata commented Nov 12, 2019

I guess something changed somewhere --- now either with TERM=linux or without it, the behavior of the show is fine.

@efiop
Copy link
Contributor

efiop commented Nov 12, 2019

@drorata Interesting. Well, something is definitely going on with the ncurses. Maybe you've indirectly updated it since or something and that resolved it. Let's close this issue for now, please feel free to reopen if you run into this issue again. Thanks for the feedback! 🙂

@efiop
Copy link
Contributor

efiop commented Nov 17, 2019

@drorata FYI: #2807 .

@efiop
Copy link
Contributor

efiop commented Dec 11, 2019

@drorata 0.75.0 is now using pager instead of our own solution. Please feel free to try it out 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Did we break something? p2-medium Medium priority, should be done, but less important research
Projects
None yet
Development

No branches or pull requests

4 participants