-
Notifications
You must be signed in to change notification settings - Fork 90
Set IJulia.stdio_bytes
in a forward-compatible way
#353
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
Conversation
This might not be the right time/place to suggest, but this kind of interaction and issue would be better solved by adding an IJulia extension, which would solve the compat aspect of this. |
That's true, this is all I have time for ATM but yeah ideally both IJulia and Distributed support would be in extensions. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #353 +/- ##
==========================================
+ Coverage 93.48% 96.43% +2.95%
==========================================
Files 1 1
Lines 399 561 +162
==========================================
+ Hits 373 541 +168
+ Misses 26 20 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ProgressMeter.jl has a lot of these, which look difficult to move into extensions without type-piracy: function printover(io::IO, s::AbstractString, color::Symbol = :color_normal)
print(io, "\r")
printstyled(io, s; color=color)
if isdefined(Main, :IJulia)
Main.IJulia.stdio_bytes[] = 0 # issue #76: circumvent IJulia I/O throttling
elseif isdefined(Main, :ESS) || isdefined(Main, :Atom)
else
print(io, "\u1b[K") # clear the rest of the line
end
end an extension for Distributed.jl looks more doable |
I've changed this to use a new public function that will be used: https://julialang.github.io/IJulia.jl/dev/library/public/#IJulia.reset_stdio_count I think it's ready to be merged now that the IJulia PR has been merged. |
should we first add a version of ProgressMeter that has a |
We could, but I don't think that's necessary with the current code? Or are you saying that we make one release with the current code and a weakdep for IJulia 1.29, and then another version with the new code I added and a weakdep on IJulia 1.30? |
yes that's what I meant |
Come to think of it I'm actually not sure that would work in a lot of cases because of how weird IJulia is 😅 Many users install it in their global environment instead of in notebook environments, so someone could be in a situation where the global environment has an old IJulia version and new ProgressMeter version. AFAIK compat bounds aren't always respected with stacked environments so that would cause an error if ProgressMeter only supported the newer IJulia version (and vice-versa). |
The property will change to a field of a struct in an upcoming release.
(minor change in f3a99ae to make the code use |
@@ -617,7 +617,12 @@ function printover(io::IO, s::AbstractString, color::Symbol = :color_normal) | |||
print(io, "\r") | |||
printstyled(io, s; color=color) | |||
if isdefined(Main, :IJulia) | |||
Main.IJulia.stdio_bytes[] = 0 # issue #76: circumvent IJulia I/O throttling | |||
# issue #76: circumvent IJulia I/O throttling | |||
if pkgversion(Main.IJulia) < v"1.30" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this have used isdefined(Main.IJulia, :reset_stdio_count)
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that would work too, I just prefer this style.
The property will change to a field of a struct in an upcoming release: JuliaLang/IJulia.jl#1145