Skip to content

LogReader: remove commaCarSegments selector #35750

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

sshane
Copy link
Contributor

@sshane sshane commented Jul 18, 2025

Added in #31169. I understand this was done to speed up log reading for our contributors by not trying every source before this, but we really shouldn't have done this. This starts to pollute the LogReader and make it much less generic/easy to maintain. This should have sparked auto_source API improvements instead.

After #35749 you can now just do this to get nearly the full speed back:

LogReader(db_route, sources=[comma_car_segments_source])

@github-actions github-actions bot added the tools label Jul 18, 2025
@@ -9,7 +9,7 @@ class RE:

INDEX = r'-?[0-9]+'
SLICE = fr'(?P<start>{INDEX})?:?(?P<end>{INDEX})?:?(?P<step>{INDEX})?'
SEGMENT_RANGE = fr'{ROUTE_NAME}(?:(--|/)(?P<slice>({SLICE})))?(?:/(?P<selector>([qras])))?'
SEGMENT_RANGE = fr'{ROUTE_NAME}(?:(--|/)(?P<slice>({SLICE})))?(?:/(?P<selector>([qra])))?'
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to update the dataset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have permission to push to the hugging face

@sshane sshane changed the title a source should not be a readmode... and readmode should be logtype LogReader: remove commaCarSegments selector Jul 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants