-
Notifications
You must be signed in to change notification settings - Fork 40
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
Refactor the codebase #14
Conversation
SachiraKuruppu
commented
Sep 13, 2023
- Extract the RAG e2e training script process_function into utils -- To declutter the training script.
- Rename train.py module in utils folder to train_utils.py -- Make it clear that these are util functions.
- Move base models out of the training folder to a separate folder -- These models are used in evaluation too. Not ideal to keep them in the training folder.
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.
can we remove unused imports :)
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.
let's change the name of this : peft_lora_constrastive_learning.py -->> retriever_only.py
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.
Renamed this to train_retriever_only.py to be consistent with the rag_e2e training script.
82a5ef1
to
587e2d6
Compare
Unused imports will cause tests to fail, so we should be okay there |
@@ -13,13 +13,18 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
import os | |||
import sys | |||
|
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.
This should not be necessary if you are installing the package. Can we remov this?
@rsachira can you remove the line
Then it won't be necessary |