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

fix: additional fixes from JET, expand the JET check #12

Merged
merged 4 commits into from
Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,10 @@ jobs:
env:
JULIAHUB_TEST_JET: basic
continue-on-error: true
- name: "JET.test_package(..., mode=:typo)"
- name: "JET.test_package() w/ custom filtering"
run: julia --color=yes --project=@juliahub-jet test/jet.jl
env:
JULIAHUB_TEST_JET: custom-filtering

docs:
name: Documentation
Expand Down
5 changes: 3 additions & 2 deletions src/jobs/jobs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -349,12 +349,13 @@ function job(id::AbstractString; throw::Bool=true, auth::Authentication=__auth__
r.status == 200 || _throw_invalidresponse(r)
job, json = _parse_response_json(r, Dict)
details = get(job, "details") do
throw(JuliaHubError("Invalid JSON returned by the server:\n$(json)"))
Base.throw(JuliaHubError("Invalid JSON returned by the server:\n$(json)"))
end
if isempty(details)
return _throw_or_nothing(; msg="Job '$(id)' does not exist.", throw)
end
length(details) > 1 && throw(JuliaHubError("Invalid JSON returned by the server:\n$(json)"))
length(details) > 1 &&
Base.throw(JuliaHubError("Invalid JSON returned by the server:\n$(json)"))
return Job(only(details))
end

Expand Down
4 changes: 2 additions & 2 deletions src/jobs/logging-kafka.jl
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ _job_logs_active_logs_view(b::KafkaLogsBuffer) = @view(b._logs[b._active_range])

_kafka_next_offset(b::KafkaLogsBuffer) = isempty(b._logs) ? 0 : (last(b._logs)._offset + 1)

function _job_logs_kafka_newer!(
function _job_logs_newer!(
auth::Authentication, b::KafkaLogsBuffer; count::Union{Integer, Nothing}=nothing
)
# If there is a streaming task running, then this will be a no-op
Expand Down Expand Up @@ -209,7 +209,7 @@ function _kafka_update_active_range!(b::KafkaLogsBuffer; start=nothing, stop=not
return nothing
end

function _job_logs_kafka_older!(
function _job_logs_older!(
auth::Authentication, b::KafkaLogsBuffer; count::Union{Integer, Nothing}=nothing
)
@assert isnothing(count) || count > 0
Expand Down
2 changes: 1 addition & 1 deletion src/jobs/logging-legacy.jl
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ function _job_logs_newer!(
end
# So to handle this case, we need to start fetching from the newest message and
# go all the way back to the very first message.
_job_logs_legacy_fill_buffer!(buffer)
_job_logs_legacy_fill_buffer!(auth, buffer)
# If the buffer is still empty, we no-op return again. This means that the job
# still hasn't produced any logs.
isempty(buffer._logs) && return nothing
Expand Down
21 changes: 7 additions & 14 deletions src/jobs/logging.jl
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,11 @@ function Base.show(io::IO, log::JobLogMessage)
print(io, "JobLogMessage(ZonedDateTime(\"$(log.timestamp)\"), \"$(log.message)\", ...)")
end

_print_log_list(logs::AbstractVector; kwargs...) = _print_log_list(stdout, logs; kwargs...)
function _print_log_list(io::IO, logs::AbstractVector; all=false, nlines=_default_display_lines(io))
_print_log_list(logs::Vector{JobLogMessage}; kwargs...) = _print_log_list(stdout, logs; kwargs...)
function _print_log_list(
io::IO, logs::Vector{JobLogMessage}; all::Bool=false,
nlines::Integer=_default_display_lines(io)
)
@assert Base.all(x -> isa(x, JobLogMessage), logs) # note: kwarg shadows function
@assert nlines >= 1
isempty(logs) && return nothing
Expand Down Expand Up @@ -282,12 +285,7 @@ function job_logs_older!(
auth::Authentication=__auth__(),
)::AbstractJobLogsBuffer
lock(buffer) do
# TODO: use multiple dispatch here
if _job_logging_api_version(auth, buffer._jobname) == _KafkaLogging()
_job_logs_kafka_older!(auth, buffer; count)
else
_job_logs_older!(auth, buffer; count)
end
_job_logs_older!(auth, buffer; count)
end
return buffer
end
Expand Down Expand Up @@ -315,12 +313,7 @@ function job_logs_newer!(
auth::Authentication=__auth__(),
)::AbstractJobLogsBuffer
lock(buffer) do
# TODO: use multiple dispatch here
if _job_logging_api_version(auth, buffer._jobname) == _KafkaLogging()
_job_logs_kafka_newer!(auth, buffer; count)
else
_job_logs_newer!(auth, buffer; count)
end
_job_logs_newer!(auth, buffer; count)
end
return buffer
end
34 changes: 32 additions & 2 deletions test/jet.jl
Original file line number Diff line number Diff line change
@@ -1,9 +1,39 @@
import JET, JuliaHub
using Test

jet_mode = Symbol(get(ENV, "JULIAHUB_TEST_JET", "typo"))
jet_mode = get(ENV, "JULIAHUB_TEST_JET", "typo")
@info "Running JET.jl in mode=:$(jet_mode)"

# The following filter is used when jet_mode == "custom-filtering"
struct JuliaHubReportFilter end
function JET.configured_reports(::JuliaHubReportFilter, reports::Vector{JET.InferenceErrorReport})
filter(reports) do report
# This is necessary since a custom `report_config` overrides `target_defined_modules` setting
occursin("JuliaHub", string(last(report.vst).linfo.def.module)) || return false
# We'll ignore all the union split errors, since they are generally actually valid cases
# where simply an isnothing() check is not being inferred correctly
if isa(report, JET.MethodErrorReport) && report.union_split > 1
return false
end
# The Kafka code (which is currently not enabled) has a few bugs that JET actually
# reveals. But we ignore them for now.
endswith(string(report.vst[end].file), "logging-kafka.jl") && return false
# We also ignore the _restput_mockable() error in restapi.jl, since JET seems to
# assume that kwargs... must be non-empty
contains(string(report.vst[end].linfo.def.name), "_restput_mockable") && return false
return true
end
end

@testset "JET" begin
JET.test_package("JuliaHub"; target_defined_modules=true, mode=jet_mode)
if jet_mode == "custom-filtering"
JET.test_package(
"JuliaHub"; report_config=JuliaHubReportFilter(),
toplevel_logger=nothing,
)
else
JET.test_package(
"JuliaHub"; target_defined_modules=true, mode=Symbol(jet_mode), toplevel_logger=nothing
)
end
end
Loading