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

Removed Javis Live Viewer from Core Javis #298

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheCedarPrince
Copy link
Member

PR Checklist

If you are contributing to Javis.jl, please make sure you are able to check off each item on this list:

  • Did I update CHANGELOG.md with whatever changes/features I added with this PR?
  • Did I make sure to only change the part of the file where I introduced a new change/feature?
  • Did I cover all corner cases to be close to 100% test coverage (if applicable)?
  • Did I properly add Javis dependencies to the Project.toml + set an upper bound of the dependency (if applicable)?
  • Did I properly add test dependencies to the test directory (if applicable)?
  • Did I check relevant tutorials that may be affected by changes in this PR?
  • Did I clearly articulate why this PR was made the way it was and how it was made?

Link to relevant issue(s)
Closes #297

How did you address these issues with this PR? What methods did you use?

In this, I removed usage of the Javis Live Viewer. It does indeed increase speed but @Wikunia and I will have to determine how best to handle where to put the Live Viewer.

@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #298 (27be30e) into master (6f97cca) will decrease coverage by 5.61%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
- Coverage   95.93%   90.31%   -5.62%     
==========================================
  Files          21       21              
  Lines        1008     1022      +14     
==========================================
- Hits          967      923      -44     
- Misses         41       99      +58     
Impacted Files Coverage Δ
src/Javis.jl 99.10% <ø> (+1.71%) ⬆️
src/javis_viewer.jl 0.00% <0.00%> (-97.73%) ⬇️

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 6f97cca...27be30e. Read the comment docs.

@agerlach
Copy link
Contributor

agerlach commented Dec 4, 2020

For the headless application discussed in #297, although heavy, the dependencies on Gtk and GtkReative is not the core issue. The issues only arises on precompilation

julia> using Javis
[ Info: Precompiling Javis [78b212ba-a7f9-42d4-b726-60726080707e]
ERROR: LoadError: InitError: Cannot open display: 
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] Gtk.GLib.GError(::Gtk.var"#304#310") at /home/username/.julia/packages/Gtk/C22jV/src/GLib/gerror.jl:17
 [3] __init__() at /home/username/.julia/packages/Gtk/C22jV/src/Gtk.jl:129
 [4] _include_from_serialized(::String, ::Array{Any,1}) at ./loading.jl:697
 [5] _require_search_from_serialized(::Base.PkgId, ::String) at ./loading.jl:782
 [6] _require(::Base.PkgId) at ./loading.jl:1007
 [7] require(::Base.PkgId) at ./loading.jl:928
 [8] require(::Module, ::Symbol) at ./loading.jl:923
 [9] include(::Function, ::Module, ::String) at ./Base.jl:380
 [10] include(::Module, ::String) at ./Base.jl:368
 [11] top-level scope at none:2
 [12] eval at ./boot.jl:331 [inlined]
 [13] eval(::Expr) at ./client.jl:467
 [14] top-level scope at ./none:3
during initialization of module Gtk
in expression starting at /home/username/.julia/dev/Javis/src/Javis.jl:20
ERROR: Failed to precompile Javis [78b212ba-a7f9-42d4-b726-60726080707e] to /home/username/.julia/compiled/v1.5/Javis/NRkHq_RgONx.ji.
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] compilecache(::Base.PkgId, ::String) at ./loading.jl:1305
 [3] _require(::Base.PkgId) at ./loading.jl:1030
 [4] require(::Base.PkgId) at ./loading.jl:928
 [5] require(::Module, ::Symbol) at ./loading.jl:923

As a stop-gap would it be possible to keep the dependency put the javis viewer in a separate or sub-module in the same repo and essentially ducktype the _javis_viewer call in render, E.g.

using Javis
...
render(..., liveviewer=true) # fail

vs.

using Javis
using Javis.LiveViewer
...
render(..., liveviewer=true) # works

I don't know if this can be managed though, I tried creating a sub-module like:

module Javis
using ... # all but Gtk and GtkReactive

module JavisViewer
    using Gtk
    using GtkReactive
    export _javis_viewer
    include("javis_viewer.jl")
end
...

But using Gtk still gets touched during precompilation leading to the same error as above.

@TheCedarPrince
Copy link
Member Author

TheCedarPrince commented Dec 14, 2020

@agerlach - yea, I thought similarly as you that if we put the code in a separate submodule, the problem would go away. Instead, it did not. There had been discussion on our side for a while about developing a separate Javis package to host all the viewing tools as this issue, which we initially thought was rather straightforward to solve, has been proving to be just the opposite. Did this edited version of Javis work for you?

@agerlach
Copy link
Contributor

@TheCedarPrince The edited version of Javis works great. Thanks.

@TheCedarPrince
Copy link
Member Author

Perfect @agerlach! At the moment, this is the best fix available for your use case. However, in the future, we will be moving Javis's viewer support to another package. In the interim, if you run into any more difficulty, please let us know on the issue that you opened or in this branch here, further. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Javis on headless servers
2 participants