Skip to content

Conversation

@ZachNagengast
Copy link
Collaborator

There are a few PR's relating to this issue already #281 #260, but this PR simply reverts the change that ignores explicitly setting useOfflineMode in constrained or expensive network conditions to its previous logic (noted here #280 (comment)). Now if useOfflineMode is set it will always be respected, and only if nil will check the monitor for the proper fallback behavior.

In addition to that, it relaxes the default connectivity to assume true until updated by the monitor. This will match the other defaults that were optimistic already:

/// Assume best case connection until updated by the monitor
public var isConnected: Bool = true <- updated from false
public var isExpensive: Bool = false
public var isConstrained: Bool = false

There is still the case where we need a faster initial update, which I believe #259 covers well.

Regarding isExpensive and isConstrained, I think they are less relevant to this library and would be better off left to the developer since both cases would still be able to download a model, only isConnected will require a different code path to potentially succeed - developers can make the decision with their own monitor and UI that would warn the user, e.g. give them the option to continue if they're ok with either case. Future work here could be surfacing that to developers without throwing when there technically is connectivity.

Comment on lines +579 to +581
let shouldUseOfflineMode = await NetworkMonitor.shared.state.shouldUseOfflineMode()

if useOfflineMode ?? shouldUseOfflineMode {
Copy link
Member

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!

Comment on lines -825 to +828
return !isConnected || isExpensive || isConstrained
return !isConnected
Copy link
Member

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

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.

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.

macOS 26 user unable to download models with error "offline mode: Repository not available locally"

3 participants