Skip to content

Optimistic default connectivity, respect useOfflineMode if set #286

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

Merged
merged 2 commits into from
Nov 5, 2025
Merged

Optimistic default connectivity, respect useOfflineMode if set #286

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
346f850
Assume connected by default until updated by network monitor
ZachNagengast Nov 3, 2025
c365e88
Format
ZachNagengast Nov 3, 2025
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
9 changes: 6 additions & 3 deletions Sources/Hub/HubApi.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,9 @@ public extension HubApi {
.appendingPathComponent("huggingface")
.appendingPathComponent("download")

if await NetworkMonitor.shared.state.shouldUseOfflineMode() || useOfflineMode == true {
let shouldUseOfflineMode = await NetworkMonitor.shared.state.shouldUseOfflineMode()

if useOfflineMode ?? shouldUseOfflineMode {
Comment on lines +579 to +581
Copy link
Member

@pcuenca pcuenca Nov 4, 2025

Choose a reason for hiding this comment

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

Yes, I think you and @DePasqualeOrg in #281 are correct here!

if !FileManager.default.fileExists(atPath: repoDestination.path) {
throw EnvironmentError.offlineModeError(String(localized: "Repository not available locally"))
}
Expand Down Expand Up @@ -808,7 +810,8 @@ public extension HubApi {
/// Network monitor helper class to help decide whether to use offline mode
extension HubApi {
private actor NetworkStateActor {
public var isConnected: Bool = false
/// Assume best case connection until updated by the monitor
public var isConnected: Bool = true
public var isExpensive: Bool = false
public var isConstrained: Bool = false

Expand All @@ -822,7 +825,7 @@ extension HubApi {
if ProcessInfo.processInfo.environment["CI_DISABLE_NETWORK_MONITOR"] == "1" {
return false
}
return !isConnected || isExpensive || isConstrained
return !isConnected
Comment on lines -825 to +828
Copy link
Member

@pcuenca pcuenca Nov 4, 2025

Choose a reason for hiding this comment

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

I don't have a strong opinion here, I see how it would be the responsibility of the user app whether to allow to proceed. But they can also force behaviour with useOfflineMode, can't they?

Copy link
Collaborator Author

@ZachNagengast ZachNagengast Nov 4, 2025

Choose a reason for hiding this comment

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

Yes, this is just a bit more lenient default, like @DePasqualeOrg mentioned the default when on a cell connection with expensive bandwidth is to throw, which would definitely be one way to surface the issue to the user - but I'm hoping this is a bit less opinionated / more lenient to reduce any surprises without solid documentation on when and where "non-fatal" network based throws will occur, i.e. the download is only technically impossible when there is no network connection at all.

}
}

Expand Down