Skip to content

fix(llm): avoid generating config twice#56

Merged
imbajin merged 5 commits intoapache:mainfrom
jiajunly:config-update
Jul 31, 2024
Merged

fix(llm): avoid generating config twice#56
imbajin merged 5 commits intoapache:mainfrom
jiajunly:config-update

Conversation

@jiajunly
Copy link
Copy Markdown
Contributor

@jiajunly jiajunly commented Jul 28, 2024

Add an update option to let users choose to fix this.

@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jul 28, 2024
@github-actions github-actions Bot added the llm label Jul 28, 2024
@dosubot dosubot Bot added the bug Something isn't working label Jul 28, 2024
Comment thread hugegraph-llm/src/hugegraph_llm/config/generate.py
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jul 29, 2024
if __name__ == '__main__':
parser = argparse.ArgumentParser(description='Generate hugegraph-llm config file')
settings.generate_env()
parser.add_argument("-U", "--update", action="store_true", help="Update the config file")
Copy link
Copy Markdown
Member

@imbajin imbajin Jul 29, 2024

Choose a reason for hiding this comment

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

maybe -u is more user-friendly? (if no conflicts found)

Also a little confused, update. Why do .env files need to be scripted separately? Isn't the (configs)change application checked by default? @ChenZiHong-Gavin

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Personally, I think that separation helps to better prompt users to check or modify their configurations. But update is more like an overwrite operation. Would it be better to change to -o and --overwrite?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

maybe -u is more user-friendly? (if no conflicts found)

Also a little confused, update. Why do .env files need to be scripted separately? Isn't the (configs)change application checked by default? @ChenZiHong-Gavin

Yes, every time Config is initialized, it automatically checks and generates the .env file. This script is a rewrite of the original script that generated the JSON file, and if we don't think it's necessary, it can actually be deleted.

@imbajin imbajin changed the title fix(hugegraph-llm): Avoid generating config twice fix(llm): avoid generating config twice Jul 29, 2024
@imbajin
Copy link
Copy Markdown
Member

imbajin commented Jul 29, 2024

Also, seems the CI failed now? (any context for it?)

image

@jiajunly
Copy link
Copy Markdown
Contributor Author

Also, seems the CI failed now? (any context for it?)

image

It looks like a problem with gradio dependencies. I'll try changing the version.

Comment thread hugegraph-llm/requirements.txt Outdated
@jiajunly jiajunly requested a review from imbajin July 31, 2024 09:29
@imbajin imbajin merged commit e7d8f56 into apache:main Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working llm size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants