Skip to content
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

reduction_factor -> bottleneck_size #14

Closed
wants to merge 1 commit into from
Closed

Conversation

calpt
Copy link
Member

@calpt calpt commented Jun 30, 2020

@calpt calpt requested a review from JoPfeiff June 30, 2020 17:52
Copy link
Member

@JoPfeiff JoPfeiff 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, but requires manual changes in nothing_to_see_here? First add a PR there and then we merge simultaneoulsy?

@calpt
Copy link
Member Author

calpt commented Jul 7, 2020

looks good, but requires manual changes in nothing_to_see_here? First add a PR there and then we merge simultaneoulsy?

We would require a manual change of all currently uploaded adapters (info card + zip folder on the server)

@JoPfeiff
Copy link
Member

JoPfeiff commented Jul 7, 2020

Yeah thats what I thought.
Alternatively, how much effort does it involve to have both settings available?
A user is required to set either? In the frontend we would present the bottleneck size.

@calpt
Copy link
Member Author

calpt commented Jul 7, 2020

That would certainly be possible for the yaml files. Not sure how much overhead it causes on the library side as we would have to process configs with different keys but same semantics (could be a problem for hashing, resolving etc).

@JoPfeiff
Copy link
Member

JoPfeiff commented Jul 7, 2020

makes sense.
To bypass that we could always translate the reduction_factor into bottleneck_size.
This is always possible, however not the other way around:

hidden_size = 768
reduction_factor = 32 => bottleneck_size = 24

however

hidden_size = 768
bottleneck_size = 100 => reduction_factor = 7.68 

we would not want to store floats here...

We could thus only hash the bottleneck_size?

@calpt
Copy link
Member Author

calpt commented Jul 7, 2020

yes, I think something like this would be possible.

just for clarification: we now always assume a hidden size of 768, right? So the conversion would be reduction_factor = 768 / bottleneck_size regardless of the actual model used (compared to previously, where the reduction factor was dependent on the actual hidden size).

@JoPfeiff
Copy link
Member

JoPfeiff commented Jul 7, 2020

No, the reduction_factor is always and remains dependent on the actual hidden_size.
So for Base the hidden_size = 768, for Large its hidden_size = 1024 (I think).
If reduction_factor is set, the bottleneck_size should be calculated according to the actual hidden_size

@JoPfeiff
Copy link
Member

JoPfeiff commented Jul 9, 2020

this has not been implemented yet correct?

@calpt
Copy link
Member Author

calpt commented Jul 9, 2020

Not yet, I'll try to do it as soon as possible (but probably not before the weekend 😅)

@JoPfeiff
Copy link
Member

JoPfeiff commented Jul 9, 2020

No problem at all, I just had the PR set for approved and was worried that someone will merge it :)

@JoPfeiff JoPfeiff self-requested a review July 9, 2020 10:27
@calpt
Copy link
Member Author

calpt commented Jul 13, 2020

@JoPfeiff how important is the implementation of this change at the moment? Imho a "clean" solution supporting both options is a bit tricky.

@JoPfeiff
Copy link
Member

not a priority. we can get back to it later

@calpt calpt force-pushed the master branch 2 times, most recently from 6245d31 to d9c06ec Compare January 6, 2022 19:43
@calpt calpt force-pushed the master branch 3 times, most recently from 0399280 to 675d10e Compare January 31, 2022 22:27
@calpt calpt force-pushed the master branch 2 times, most recently from 97bf6d6 to 4c34dd0 Compare May 30, 2022 16:01
calpt pushed a commit that referenced this pull request Aug 25, 2023
calpt pushed a commit to calpt/adapters that referenced this pull request Aug 28, 2023
@calpt calpt closed this Nov 20, 2023
@calpt calpt deleted the dev/bottleneck_size branch November 23, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reduction factor refactor to bottleneck_size
2 participants