Skip to content

"Hey comma" to bookmark segments #35732

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 27 commits into
base: master
Choose a base branch
from
Open

"Hey comma" to bookmark segments #35732

wants to merge 27 commits into from

Conversation

Quantizr
Copy link
Contributor

Saying "Hey comma" will let you bookmark a segment, in the same way that clicking the bookmark button in the sidebar currently does.

@Quantizr
Copy link
Contributor Author

Quantizr commented Jul 15, 2025

things im a little iffy on

  • apache attribution?
  • what core to set config_realtime_process
  • naming feedbackd vs something else like bookmarkd?
  • should this even be in modeld?

@Quantizr
Copy link
Contributor Author

I trolled and I thought the bad cores were the high index ones...
so actual CPU usage was 52% on the bad cores

quantized the embedding model to fp16, tinygrad LLVM has worse performance than fp32 on the bad core, so switched backend to CPU=1 and gets 42% CPU usage on bad cores

(however on the good core, LLVM seems to do better than CPU with the quantized model???)

there also seems to be possibly be some performance gap depending on if the net was compiled on the same type of core it is run on

@Quantizr
Copy link
Contributor Author

Quantizr commented Jul 15, 2025

17-18% little core CPU usage
roughly 2.5ms GPU usage / 80ms, 31 ms GPU / second, 3.1% GPU usage

@Quantizr
Copy link
Contributor Author

IMAGE=2 compiles the model to be ~ same time as BEAM=1 so this the previous cpu/gpu usage is the same, and compile times are normal

17-18% little core CPU usage
roughly 2.5ms GPU usage / 80ms, 31 ms GPU / second, 3.1% GPU usage

@Quantizr Quantizr requested a review from adeebshihadeh July 16, 2025 07:41
Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

Did a quick pass, looking into openWakeWord now

cereal/log.capnp Outdated
wakewordExecutionTime @3 :Float32;
totalExecutionTime @4 :Float32;

wakewordProb @5 :Float32;
Copy link
Contributor

Choose a reason for hiding this comment

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

float16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

float16 isn't natively supported in capnp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how much do we care about making it small? I could scale and cast to UInt16 and probably UInt8 for wakewordProb

Comment on lines 40 to 42
if sm.updated['bookmarkButton'] and (current_time - last_user_flag_time) > debounce_period:
cloudlog.info("Bookmark button pressed!")
should_send_flag = True
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't read this here. the backend should just handle both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how you wanted to handle the bookmark button, but if we wanted to have the bookmark button also record audio feedback in the future, I think it would still make sense to handle both in feedbackd.

@commaai commaai deleted a comment from commaci-public Jul 17, 2025
@commaai commaai deleted a comment from github-actions bot Jul 17, 2025
@Quantizr Quantizr requested a review from adeebshihadeh July 17, 2025 04:02
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