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.

Sign up for GitHub

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

Jump to bottom

provide a scratchspace for AtlasRep data if necessary #1026

Closed
wants to merge 1 commit into from
Closed

provide a scratchspace for AtlasRep data if necessary #1026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/GAP.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,9 @@ function __init__()
end

Packages.init_packagemanager()

# The AtlasRep package may get loaded automatically when GAP starts.
Packages.init_atlasrep()
end

"""
Expand Down
70 changes: 66 additions & 4 deletions src/packages.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module Packages

import Downloads
import Pidfile
import Scratch: @get_scratch!
import ...GAP: Globals, GapObj, replace_global!, RNamObj, sysinfo, Wrappers

const DEFAULT_PKGDIR = Ref{String}()
Expand Down Expand Up @@ -119,7 +120,7 @@ function load(spec::String, version::String = ""; install::Union{Bool, String} =
warning_level_orig = Wrappers.InfoLevel(Globals.InfoWarning)
Wrappers.SetInfoLevel(Globals.InfoWarning, 0)
end
loaded = Wrappers.LoadPackage(gspec, gversion, !quiet)
loaded = _load_internal(gspec, gversion, !quiet)
if spec_is_path
Wrappers.SetInfoLevel(Globals.InfoWarning, warning_level_orig)
end
Expand Down Expand Up @@ -172,7 +173,7 @@ function load(spec::String, version::String = ""; install::Union{Bool, String} =

# ... then try to load the package, ...
Wrappers.SetPackagePath(pkgname, gspec)
loaded = Wrappers.LoadPackage(pkgname, gversion, !quiet)
loaded = _load_internal(pkgname, gversion, !quiet)

# ..., and reinstall the old info records
# (which were removed by `Wrappers.SetPackagePath`).
Expand All @@ -191,14 +192,14 @@ function load(spec::String, version::String = ""; install::Union{Bool, String} =
# without showing messages.
if Packages.install(spec, version; interactive = false, quiet)
# Make sure that the installed version is admissible.
return Wrappers.LoadPackage(gspec, gversion, !quiet) == true
return _load_internal(gspec, gversion, !quiet) == true
end
elseif install isa String
# `Packages.install` deals with the `install` information.
if Packages.install(install, version; interactive = false, quiet)
# Make sure that the installed version is admissible
# (and that the package given by `install` fits to `spec`).
return Wrappers.LoadPackage(gspec, gversion, !quiet) == true
return _load_internal(gspec, gversion, !quiet) == true
end
end

Expand All @@ -207,6 +208,18 @@ function load(spec::String, version::String = ""; install::Union{Bool, String} =
# GAP unfortunately only gives us info messages...
end

function _load_internal(gspec::GapObj, gversion::GapObj, quiet::Bool)
loaded = Wrappers.LoadPackage(gspec, gversion, quiet)

# If the AtlasRep package is loaded afterwards (perhaps indirectly)
# then provide its scratchspace if necessary.
if atlasrep_download_cache == "" && Wrappers.IsPackageLoaded(GapObj("AtlasRep"))
init_atlasrep()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not catch the cases were a user calls GAP code that does LoadPackage("atlasrep").

I don't think we need this if we call SetUserPreference earlier and in more cases, see below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set the value of the preference before AtlasRep gets loaded then we have to set it unconditionally, and create the scratchspace even if the package will not be loaded.

I am not sure how to deal with several available package versions: As long as the package is not loaded, we do not know which version will be loaded, and there is only one path which we can set.

end

return loaded
end

"""
install(spec::String, version::String = "";
interactive::Bool = true, quiet::Bool = false,
Expand Down Expand Up @@ -374,4 +387,53 @@ function locate_package(name::String)
return String(Wrappers.ELM_REC(loaded, lname)[1])
end


##############################################################################
#
# special handling for the GAP package AtlasRep
#
# GAP.jl supports Julia artifacts for GAP packages.
# One feature of the AtlasRep package is to download data files and
# (if wanted) to store them in a local directory.
# The path of this directory is either explicitly set in the user preference
# `AtlasRepDataDirectory` of the package, or it is computed when the AtlasRep
# package gets loaded.
# The value of the user preference is an empty string if and only if
# GAP does not know a path to a writable directory for the data files,
# which means that downloaded files cannot be cached;
# in this case, GAP.jl provides a scratchspace for the data,
# and sets the value of the user preference accordingly.
# We can use this scratchspace independent of the version of Julia, GAP.jl,
# and AtlasRep.

# The path to the scratchspace, will be filled by `init_atlasrep()`
atlasrep_download_cache = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is type unstable. Better is something like this:

const atlasrep_download_cache = Ref{String}()

...

!isassigned(atlasrep_download_cache) || return

...

atlasrep_download_cache[] = String(val)

and so on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O.k.
(I had just copied the code from the example in Scratch.jl.)


# If GAP's AtlasRep package does not yet know a writable directory
# for storing downloaded data, provide a Julia scratch space.
function init_atlasrep()
atlasrep_download_cache == "" || return
atlasrep = GapObj("atlasrep")
AtlasRepDataDirectory = GapObj("AtlasRepDataDirectory")
val = Wrappers.UserPreference(atlasrep, AtlasRepDataDirectory)
if Wrappers.IsString(val)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is not a string, then I think the only other value (baring a mistake in gap.ini) is fail, which means AtlasRep is not loaded yet and the user did not set a preference.

So isn't that a situation in which we should provide a path, too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, if AtlasRep does not get loaded at all then GAP.jl need not create a scratchspace at all.
As long as the preference AtlasRepDataDirectory is not set (value fail), we need not provide a default for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But creating a scratch space is super cheap. If the price for avoiding it is complicated and error-prone logic to detect whether the scratch space is really needed, I would just forget about that and always create the scratch space.

# A default is set.
if length(val) == 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some tests with plain GAP locally: if I have no AtlasRepDataDirectory user pref set, and start a regular GAP session which auto-loads atlasrep, then AtlasRepDataDirectory is set to a default value (a path inside the atlasrep package directory).

So if I read it right, then with atlasrep autoloaded (which is the case right now with GAP.jl), this code would be never triggered?!

Can we perhaps instead somehow distinguish between "the user set a value (so honor that)" and "some kind of default value that we should override". E.g. perhaps one way to do that would be to perform just that check in gap/systemfile.g (so that it is run before any packages are loaded), i.e. basically: check if UserPreference("atlasrep", "AtlasRepDataDirectory") returns a value different from fail or not.

Anyway, another potential issue: What if we set a path to a scratch dir, but then the user calls WriteGapIniFile. Then the scratch path is put into the resulting gap.ini -- but it may be invalid the next time we run (and load gap.ini). So perhaps we need some logic that detects if the AtlasRepDataDirectory value "looks like a Julia scratch space path", and in that case, update it anyway?

In closing, I think for 99.9% of all users, they are not aware of this setting and don't care about it and with that in mind the simples solution would be to just call SetUserPreference unconditionally... but I realize this is annoying for a handful of power users (such as yourself), so I guess we can't really do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The computed default for AtlasRepDataDirectory is a path inside the package directory if this is writable, and in this case I want to keep this default.
  • Also in the case that the gap.ini file sets a path (such that no computed default gets set), I want to keep this path because the user wants it.
  • If the path is not writable (which will happen if the artifact in question is read-only --currently it seems to be writable) then the computed default is an empty string (meaning that downloaded data are not cached), and then the idea is that GAP.jl can provide a scratchspace as a way to enable caching.

Accidentally storing the path to the scratchspace in a gap.ini file is an issue, good point.
More generally, we can override any value of AtlasRepDataDirectory that is not a valid (writable) directory path.

What I had mentioned already is that the currently proposed code ignores that a user may set an empty string in order to disable caching. One solution would be to check in the case of an empty string whether the package directory is writable, and if yes to conclude that the empty string is intentional, and to keep it.

Concerning the fact that the replacement of the default does not get triggered if one starts GAP without packages and later on loads GAP packages only on the GAP side, we could argue that the situation is not worse as it was before, and if one does not use the features from the Julia side than one does not get its advantages.

(I had thought whether a clean solution would be to put the relevant Julia code into the function for the default computation in AtlasRep's PackageInfo.g file. This would remove the problem that one might load all packages only with GAP's LoadPackage. The problem with the scratchspace path stored in gap.ini cannot be solved then, but it could be documented in AtlasRep that storing the computed value is not a good idea.)

# There is no writable directory yet.
# If the scratch space is not yet there, create it and the relevant
# subdirectories.
# (The AtlasRep package is perhaps not yet loaded at this time.)
global atlasrep_download_cache = @get_scratch!("atlasrep_cache")
Wrappers.SetUserPreference(atlasrep, AtlasRepDataDirectory,
GapObj(atlasrep_download_cache))
for dir in ["datagens", "dataword", "dataext"]
fname = joinpath(atlasrep_download_cache, dir)
isdir(fname) || mkdir(fname)
end
else
global atlasrep_download_cache = String(val)
end
end
end

end
4 changes: 4 additions & 0 deletions src/wrappers.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,17 @@ import GAP: @wrap
@wrap RNamObj(x::Any)::Int
@wrap SetInfoLevel(x::GapObj, y::Int)::Nothing
@wrap SetPackagePath(x::GapObj, y::GapObj)::Nothing
@wrap SetUserPreference(x::GapObj, y::Any)::Nothing
@wrap SetUserPreference(x::GapObj, y::GapObj, z::Any)::Nothing
@wrap ShallowCopy(x::Any)::Any
@wrap String(x::Any)::Any
@wrap StringDisplayObj(x::Any)::GapObj
@wrap StringViewObj(x::Any)::GapObj
@wrap StructuralCopy(x::Any)::Any
@wrap SUM(x::Any, y::Any)::Any
@wrap UNB_REC(x::GapObj, y::Int)::Nothing
@wrap UserPreference(x::GapObj)::Any
@wrap UserPreference(x::GapObj, y::GapObj)::Any
@wrap ZeroSameMutability(x::Any)::Any

end
Loading