-
-
Notifications
You must be signed in to change notification settings - Fork 933
Progress not showing up before the last second #444
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
Comments
As far as I know, the progress line parsing is immediate, and thus will produce results in real time, similar to what you see on the command-line. However, I am interested in your findings, maybe there is something unexpected here. |
There's definitely something fishy going on, I have replaced my "progress bar" instance with the following code: and then I ran the command. I made an asciicinema output of the command to better see the pausing, followed by the output: https://asciinema.org/a/cwoo3r1gnziyxgycbybd1p485 which is not outputting at the same rate as a raw git clone command: http://asciinema.org/a/0u002zielzbpprrj6e5h20hpa I already looked at the parsing code, and I agree it's looking totally fine, and it should work as intended. So maybe, a good idea would be to try to replicate the issue through a minimal snippet, to see whether my code could be interfering (which I believe should not, but who knows?). I'll also try to add printouts in the parser to see where it's blocking. |
Ok, I've added a bunch of printouts, along with a timer to find where the pausing is happening (cf below). So it's pretty obvious that the pausing is happening in
and now, it's pretty clear what's happening, on a file object
which is the right way to iterate line by line over a file object. full log:
|
Great debugging and findings ! I just pushed the fix ! And thanks a lot ! |
Well, it's actually still not very smooth: https://asciinema.org/a/55m8ugnq1j43k69keiasnjzoo and, as I am using master's HEAD, there's an issue with it showing:
I guess it's not relevant to the issue at hand, though. Then, I ran it again with the fix and timers in place, and here's the output:
What I'm thinking is that there's another issue here, and it's that the python file object might be iterating over |
Maybe ioctl can be used to configure the handle after it was created. Or there is a way to do that where the process is spawned. |
I think I'm onto something, I stopped with pdb to watch at the type of Then I went on to read subprocess documentation where it says:
so I guess it's worth trying with |
yay, it's what was the issue \o/ http://asciinema.org/a/5n0tyv6jmvu14u8lsi18f53c5 there's a bunch of caveats, though, because now |
Looks very good ! I am currently experimenting with adding this option just for fetch/push operations, and it doesn't seem too straightforward for some reason. Let's see how it goes. |
Though, as a side effect the fetching is quite terribly slow, certainly of the overhead of going twice to stdout. |
Why is it going through stdout twice ? And even if so, it's just a few hundred lines at most, and I wouldn't expect this to be noticeable. If it is, it might be something with regular expressions being slow ? Just guessing though. |
well, from git's stdout to GitPython, and from GitPython to stdout! |
That is something I would then consider a bug in either codebase, and hopefully fixable. |
here's a summary of my patch:
about the other errors, I just found out that I fscked up my local workspace, so to make a proper commit I need to do some cleaning first :) |
That way, real-time parsing of output should finally be possible. Related to #444
I just pushed the universal_newline fix for remote operations. It should work, but please verify. For anything, I'd be happy to merge PRs you send this way. Thanks for the asciicinema videos, it was nice to see the results of this work, and certainly helped my motivations :) ! |
I think this is fixed now: Line 553 in 902679c
|
ok, it looks like the "slowness" of my pulls might be caused by github throttling me, as I'm hammering them a bit too much:
as I'm on a gigabit connection, I'm used to the clone being a lot less :) |
@Byron I thought this was fixed. I'm not seeing the problem any more. Can this issue me closed? |
worksforme ☺ |
We can close it then, fixed under the milestone it is currently under! (do you want to push any AUTHOR changes?) |
sure thanks, I'll do that in a currently pending PR I'm pushing! |
Not sure if I should file a new bug or resurrect this one, but an earlier comment says:
Was cloning just overlooked? |
@mrozekma I believe creating a new issue would be the way to go, while assuring the issue can be reproduced with the latest version. |
I've implemented the
Progress
class to implement a nice progress bar:I'm then using it, for example here:
And then, when I clone a very large repository (like mozilla's gecko):
the progress bar is "progressing" (i.e. incrementing from 0 to 100%), but it shows up only at the very end of the fetching, after hanging for a long while. I started to debug it, and it looks like it's related to subprocess stuff.
Though, I might be implementing it wrong, which is why I'm asking here while I'm still debugging that issue.
The text was updated successfully, but these errors were encountered: