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

Resolve artifact ambiguities using shortest match #48749

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

staticfloat
Copy link
Sponsor Member

This changes the behavior of select_platform() to resolve ambiguities using a two-step process. Previously, we would simply sort the resultant triplets and return the last one (so as to provide asemi- arbitrary stability, and also to prefer higher version numbers over lower version numbers in tags). However, with the proliferation of tags (and especially the new sanitize=memory tags) we need a way to exclude these "extra" tags when our HostPlatform() does not have them.

This new matching algorithm excludes candidates from matching with the platform if there are other candidates with fewer total tags. This results in a "simplest match first" behavior, which better represents the intent of tags in the first place.

@staticfloat
Copy link
Sponsor Member Author

Pkg.jl test failure fixed in: JuliaLang/Pkg.jl#3384

@staticfloat staticfloat added the backport 1.9 Change should be backported to release-1.9 label Feb 21, 2023
@giordano
Copy link
Contributor

Would this solve ziotom78/Ducc0.jl#4 with JuliaRegistries/General#78531?

@staticfloat
Copy link
Sponsor Member Author

staticfloat commented Feb 26, 2023

Yes, it would allow a default match.

For the particular case of march though, I think we could also build a comparison strategy for the march key that does something like this:

using Base.BinaryPlatforms, Base.BinaryPlatforms.CPUID, Test

# Build mapping of march strings (assuming all march names are unique across all archs)
# to sets of strings they are compatible with.
const march_compatibility = Dict{String,Set{String}}()
for (arch, marchs) in CPUID.ISAs_by_family
    for (march, isa) in marchs
        march_compatibility[march] = Set{String}()
        for (m, i) in marchs
            if i <= isa
                push!(march_compatibility[march], m)
            end
        end
    end
end
function compare_march(a::String, b::String, a_requested::Bool, b_requested::Bool)
    # If both a and b requested, fall back to simple equality
    if a_requested && b_requested
        return a == b
    end

    # Otherwise, say that the comparison matches as long as the two 
    if a_requested
        return a  keys(march_compatibility) && b  march_compatibility[a]
    else
        return b  keys(march_compatibility) && a  march_compatibility[b]
    end
end

Here's a simple test suite:

host = HostPlatform()
host["march"] = "haswell"
Base.BinaryPlatforms.set_compare_strategy!(host, "march", compare_march)

p = Platform(arch(host), os(host); :march => "skylake")
@test !platforms_match(host, p)
@test !platforms_match(p, host)

p = Platform(arch(host), os(host); :march => "haswell")
@test platforms_match(host, p)
@test platforms_match(p, host)

p = Platform(arch(host), os(host); :march => "nehalem")
@test platforms_match(host, p)
@test platforms_match(p, host)

p = Platform(arch(host), os(host); :march => "x86_64")
@test platforms_match(host, p)
@test platforms_match(p, host)

p = Platform(arch(host), os(host); :march => "armv7l")
@test !platforms_match(host, p)
@test !platforms_match(p, host)

Note to self; we should change the march names generated by BB to be the same as in CPUID. That's probably my fault. :P

This was referenced Mar 3, 2023
staticfloat added a commit to JuliaPackaging/JLLPrefixes.jl that referenced this pull request Apr 3, 2023
This is a temporary workaround until
JuliaLang/julia#48749 is resolved.
@KristofferC KristofferC mentioned this pull request Apr 9, 2023
26 tasks
@KristofferC
Copy link
Sponsor Member

Bump

@staticfloat staticfloat force-pushed the sf/shortest_artifact_matching branch from a275169 to 4ce024f Compare April 10, 2023 17:03
This changes the behavior of `select_platform()` to resolve ambiguities
using a two-step process.  Previously, we would simply sort the
resultant triplets and return the last one (so as to provide asemi-
arbitrary stability, and also to prefer higher version numbers over
lower version numbers in tags).  However, with the proliferation of tags
(and especially the new `sanitize=memory` tags) we need a way to exclude
these "extra" tags when our `HostPlatform()` does not have them.

This new matching algorithm excludes candidates from matching with the
platform if there are other candidates with fewer total tags.  This
results in a "simplest match first" behavior, which better represents
the intent of tags in the first place.
@staticfloat staticfloat force-pushed the sf/shortest_artifact_matching branch from 4ce024f to 99938fc Compare April 10, 2023 22:21
@staticfloat staticfloat merged commit 0e361cf into master Apr 11, 2023
@staticfloat staticfloat deleted the sf/shortest_artifact_matching branch April 11, 2023 00:19
@giordano
Copy link
Contributor

giordano commented Apr 24, 2023

I feel like this is fundamentally flawed.

Edit: for the benefit of future readers, this PR was meant to address #42299 (comment)

@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Apr 25, 2023
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.

None yet

3 participants