Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bug fix: missing attention mask in VAE encoder in ACT policy #279
Bug fix: missing attention mask in VAE encoder in ACT policy #279
Changes from 1 commit
7692e31
a576523
722b765
ffb007a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you mentioned in your PR message, I think we can avoid adding a whole config file by doing
(and in the test)
Feels like a no-brainer to me but maybe I'm missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes in this case we still need a
act_1000_actions
config file...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @aliberts you have an opinion on this question around tests and config file.
See my
How it was tested
section in this PR description.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I meant also to change to using just the act.json file. Also happy to hear what @aliberts says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes this also doesn't work because the safetensor files overide themselves (cf my comment in the PR description. maybe a bit short) unless we either:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks. My stance on this is it's better to add test artifacts than to add config files to the source code. That can even mean just moving the yaml file to the artifacts directory. Also happy with both of your proposals. Waiting on @aliberts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO both are fine (the naming trick you went with is okay, although it adds artifacts they're relatively small). I was concerned with speed too but from what I'm seeing in CI tests it doesn't increase tests duration (cf this branch vs main)
Long-term though, I think it'll be much better to have more fine-grained tests and test individual components of the policies and do away with these artifacts, similar to what's done in transformers. My motivations for this are:
For now it's okay because this granularity allows us to iterate faster while still having guardrails. Happy to hear your opinions on this as well.