Skip to content

Conversation

dirkgr
Copy link
Member

@dirkgr dirkgr commented Mar 4, 2024

No description provided.

@dirkgr dirkgr requested review from epwalsh and 2015aroras March 4, 2024 20:02
@dirkgr dirkgr marked this pull request as ready for review March 4, 2024 20:02
Copy link
Member

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

LGTM

olmo/util.py Outdated
@@ -59,6 +59,7 @@ def __repr__(self) -> str:
class LogFilterType(StrEnum):
rank0_only = "rank0_only"
local_rank0_only = "local_rank0_only"
firehose = "firehose"
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 but maybe "all_ranks" instead of "firehose" to be more consistent with the existing names?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in dd454bd.

@dirkgr
Copy link
Member Author

dirkgr commented Mar 5, 2024

Tests fail because of a version conflict with cached_path. Cached_path can't be updated because of the script that parses the changelog. Looks like AGI has to wait another day.

@epwalsh
Copy link
Member

epwalsh commented Mar 5, 2024

Oyvind has a fix in this PR: #484

@dirkgr
Copy link
Member Author

dirkgr commented Mar 6, 2024

Oyvind has a fix in this PR: #484

This is true, but the underlying issue is an unnecessary pin in cached_path.

@dirkgr dirkgr merged commit 70ad30c into main Mar 6, 2024
@dirkgr dirkgr deleted the Firehose branch March 6, 2024 18:59
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.

3 participants