Skip to content

Conversation

joshhvulcan
Copy link
Contributor

No description provided.

Copy link
Collaborator

@favyen2 favyen2 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

load_model_and_optim_state only accepts paths on local filesystem, so it may be better to pass the original string to that function (or pass train_module_dir.path with a check that it is in fact on local filesystem).

It looks like currently (even before this PR) we just print "could not find helios encoder at ..." if the folder doesn't exist, which seems bad since we might erroneously fine-tune model without a checkpoint, but we can fix that when we figure out a better way to handle passing the Helios pre-training config+checkpoint to fine-tuning.

@joshhvulcan
Copy link
Contributor Author

@favyen2 Updated to include .path at your suggestion. I've been looking through load_model_and_optim_state() and it appears to support reading remote files so I didn't add a local file check.

@joshhvulcan joshhvulcan marked this pull request as ready for review September 4, 2025 19:56
@favyen2
Copy link
Collaborator

favyen2 commented Sep 4, 2025

@favyen2 Updated to include .path at your suggestion. I've been looking through load_model_and_optim_state() and it appears to support reading remote files so I didn't add a local file check.

Hm OK I was looking at https://github.com/allenai/OLMo-core/blob/main/src/olmo_core/io.py#L40 but I guess it may still be able to handle some protocols. Sorry in that case maybe it's better to pass the UPath or str(path) lol; because x.path will just include the path component not the protocol.

@joshhvulcan
Copy link
Contributor Author

Hm OK I was looking at https://github.com/allenai/OLMo-core/blob/main/src/olmo_core/io.py#L40 but I guess it may still be able to handle some protocols. Sorry in that case maybe it's better to pass the UPath or str(path) lol; because x.path will just include the path component not the protocol.

I think that just strips off the file://, but will leave gs:// in place. load_model_and_optim_state() uses RemoteFileSystemReader to read the file which eventually drops into this block in get_bytes_range() which operates on the gs protocol. Anyway, I'm just going to pass the str() version through since that seems the safest at this point.

@joshhvulcan joshhvulcan merged commit cb78159 into master Sep 5, 2025
4 checks passed
@joshhvulcan joshhvulcan deleted the josh/helios-upath branch September 5, 2025 16:25
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.

2 participants