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.

Sign up for GitHub

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

Jump to bottom

unify parameters and prompt with config #99

Closed
wants to merge 16 commits into from
Closed

unify parameters and prompt with config #99

wants to merge 16 commits into from

Conversation

ft2023
Copy link
Collaborator

@ft2023 ft2023 commented May 26, 2024 •
edited by lwaekfjlk
Loading

Closes #46

📑 Description

✅ Checks

  • My pull request adheres to the code style of this project
  • [✅ ] My code requires changes to the documentation
  • I have updated the documentation as required
  • [✅ ] All the tests have passed
  • [✅ ] Branch name follows type/descript (e.g. feature/add-llm-agents)
  • [✅ ] Ready for code review

ℹ Additional Information

add: yacs

@ft2023 ft2023 requested a review from lwaekfjlk May 26, 2024 12:41
@lwaekfjlk lwaekfjlk changed the title # solution for issue 46: The parameters and prompt are unified with config unify parameters and prompt with config May 26, 2024
lwaekfjlk
lwaekfjlk requested changes May 26, 2024
View reviewed changes
Copy link
Member

@lwaekfjlk lwaekfjlk left a comment

Choose a reason for hiding this comment

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

Can you add more guidance about how to use this config when running one experiments in the README?

lwaekfjlk
lwaekfjlk reviewed May 26, 2024
View reviewed changes
Copy link
Member

@lwaekfjlk lwaekfjlk left a comment

Choose a reason for hiding this comment

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

Additionally, I think when using the config, the input param of the agent_base.py should be changed, right? Is this PR finished yet or you are still working on adding those things in the agent_base.py?

lwaekfjlk
lwaekfjlk reviewed May 26, 2024
View reviewed changes
research_town/utils/all_config.yaml
related_paper_num: 10
base_llm: "mistralai/Mixtral-8x7B-Instruct-v0.1"

prompt:
Copy link
Member

Choose a reason for hiding this comment

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

Probably we should name them as prompt_template since they are not the real prompt.

lwaekfjlk
lwaekfjlk reviewed May 26, 2024
View reviewed changes
research_town/utils/all_config.yaml
Copy link
Member

Choose a reason for hiding this comment

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

I think yaml should not be put under utils, but in config folder or data folder

research_town/utils/config.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

We probably should add a separate config folder for this.

https://github.com/RUC-NLPIR/FlashRAG/tree/main/flashrag/config

Similar to this one, with a basic yaml for users and real experiment yaml in the scripts folder or data folder

@ft2023 ft2023 requested a review from lwaekfjlk May 30, 2024 15:25
@lwaekfjlk
Copy link
Member

thanks @ft2023 , i will review later.

lwaekfjlk
lwaekfjlk requested changes May 30, 2024
View reviewed changes
Copy link
Member

@lwaekfjlk lwaekfjlk left a comment

Choose a reason for hiding this comment

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

all the pycache should not exist in the PR.

research_town/__pycache__/__init__.cpython-38.pyc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why pyc file are in the github? These files should not be inlcuded.

@lwaekfjlk
Copy link
Member

this PR is deprecated. the new one appears in #150

@lwaekfjlk
Copy link
Member

the same function has been rewritten in #199. So close this PR.

@lwaekfjlk lwaekfjlk closed this Jun 3, 2024
@lwaekfjlk lwaekfjlk deleted the The-parameters-and-prompt-are-unified-with-config branch September 8, 2024 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lwaekfjlk lwaekfjlk lwaekfjlk requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ORG]: put different prompting into a config file to run experiments
2 participants
@ft2023 @lwaekfjlk