JuliaInterop / JuliaCall

Embed Julia in R
https://non-contradiction.github.io/JuliaCall/index.html
Other
267 stars 36 forks source link

Remove unused `jl_stdout_obj` import since it was removed in Julia 1.11 #237

Open JackDunnNZ opened 2 weeks ago

JackDunnNZ commented 2 weeks ago

This jl_stdout_obj doesn't seem to be used anywhere in the package, and was removed in Julia 1.11: https://github.com/JuliaLang/julia/pull/53250

Fixes #234

schlichtanders commented 2 weeks ago

Looking forward to get this merged! Currently RCall blocks me from transitioning to Julia 1.11

schlichtanders commented 1 week ago

@JackDunnNZ thank you for you work. What is needed to get this merged? I am really blocked by this and would like to speed up the process if possible

cderv commented 1 week ago

I believe we are all in wait of

And a discussion on what to do next should happen there.

Other PR like #210 are opened since more than 1 year with no reaction.

PallHaraldsson commented 1 week ago

bump. [My alternative suggestion is at JuliaLang, (partial) revert of the breaking change there.]

If this wasn't actually used, can't this be speedily merged? And presumably solving the problem (on master; and then tag a new version?).

JackDunnNZ commented 1 week ago

bump.

If this wasn't actually used, can't this be speedily merged? And presumably solving the problem (on master; and then tag a new version?).

Please read the issue linked above, @Non-Contradiction is unfortunately nowhere to be found

PallHaraldsson commented 1 week ago

I did and I realize @Non-Contradiction is absent, maybe only one(?) with merge access. I would be willing to fix this (get maintainer status), I see you @JackDunnNZ have some commits, presumably no access (nor any other contributor), why I also edited, mentioning my alternative at JulaLang.

It's bad that 1.11 broke this (possibly it wasn't documented API), though understandable PkgEval didn't catch (and thus probably thought ok); see issue I linked and commented on.

JackDunnNZ commented 1 week ago

Yes, unfortunately I have no commit access - I am merely an invested user that fixes things each time they have broken :)

fingolfin commented 6 days ago

There are other potential issues with this package. E.g. for some reason it doesn't seem to use the official Julia C headers, instead copying bits of it. This is inherently dangerous and fragile. I note that they copied a definition for struct jl_module_t. This definition is not matching the actual definition anymore. This may be OK (if they don't actually try to access any of the members of this struct), but overall I really wonder why this approach was chosen.

PallHaraldsson commented 6 days ago

My alternative fix, partial revert in JuliaLang has been approved (by you), though not yet merged.

It makes this PR redundant, and I believe fixes that package (for now).

Given that the package is doing something "inherently dangerous and fragile" it could break again later in 1.12+.

There's no way to take over a GitHub project (unless people die, maybe), wouldn't be a good policy. But in Julia registry there is I think a way to take over a project pointing to a forked project.

I don't know how things are handled in R, I'm a bit ignorant, is there some official procedure in case of maintainer disappearing?

ViralBShah commented 6 days ago

Do people have commit access here? Perhaps this package should live in JuliaInterop and recruit a larger pool of maintainers?

PallHaraldsson commented 5 days ago

Apparently nobody has commit access here, or the one or few that do are not listening (for over a year for @ChrisRackauckas' trivial (not-too badly needed? timeout) PR here: https://github.com/Non-Contradiction/JuliaCall/pull/211/files and 2+ weeks for this badly needed fix here, and they main guy abandoned his latest work since 2020).

I agree this could live in JuliaInterop (at least if R users would look there, or get linked there), that would mean forking it, but I'm thinking, in R community, would it still link here? Even changing docs is not possible (here) like only works in 1.11.2 (after my PR merged), and I'm not sure where R people get this from if not directly from Github, or what docs they would see.

This would need to change: https://cran.r-project.org/web/packages/JuliaCall/index.html

I don't know CRAN policies.

Maybe this indicates Julia isn't that much used from R, calling in that direction at least? [Neither RCall.jl to call R from Julia? Or interop that way, way more popular?]

Could/should this package, maybe not just be moved (good first step) but rather integrated into RCall.jl? I.e. like PythonCall.jl is for both directions, and Python-to-Julia direction is just a trivial addon in that package.

ViralBShah commented 5 days ago

I have emailed @Non-Contradiction requesting transfer to JuliaInterop.

Non-Contradiction commented 5 days ago

Dear all, thank you very much. After consideration, I agree with @ViralBShah and will transfer the package to JuliaInterop. I once granted commit access to some people. But it seems that they are busy too. It seems that transftering the package to JuliaInterop will allow a broader pool of maintainers and is the right choice.

Non-Contradiction commented 5 days ago

I agree that not using Julia's official header is dangerous and fragile. It really was an ad hoc approach in the early days of JuliaCall. On the R side, every C code compilation must happen at the package installation stage, otherwise compilation will happen every time at the loading of the package (which is very slow and intolerable, especially in the early days....).
Then JuliaCall basically needs to have Julia's official header with it. And also JuliaCall needed to consider that user may have different versions of Julia instead of a single version to use with JuliaCall. So I tried to use a minimal subset of header to avoid the conflicting between versions and yet have a functional package. But this approach is really fragile, and the existence of this issue proves exactly this.

ViralBShah commented 4 days ago

@Non-Contradiction I have given you owner access on JuliaInterop to move the package. Once you move, I will reduce the permission level but still make sure you have full access to your package.

Thanks for the explanation. I understand the challenge better, and others may be better suited to chime in on what can be done to improve things.