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

[Inference] Fix auth token and add models starcoder and llama2 #39

Merged
merged 33 commits into from
Feb 7, 2024

Conversation

Deegue
Copy link
Contributor

@Deegue Deegue commented Jan 8, 2024

No description provided.

carsonwang pushed a commit to carsonwang/llm-on-ray that referenced this pull request Jan 9, 2024
* add num_to_keep for pretrainers

Signed-off-by: Zhi Lin <[email protected]>

* add num_to_keep to config

Signed-off-by: Zhi Lin <[email protected]>

---------

Signed-off-by: Zhi Lin <[email protected]>
@KepingYan
Copy link
Contributor

Why is env HF_ACCESS_TOKEN removed, is it no longer needed?

@KepingYan
Copy link
Contributor

Please also help add **config.dict() at PeftModel.from_pretrained(model, model_desc.peft_model_id_or_path) and DeltaTunerModel.from_pretrained(model, model_desc.peft_model_id_or_path) in both transformer_predictor.py and deepspeed_predictor.py.

@Deegue
Copy link
Contributor Author

Deegue commented Jan 10, 2024

Please also help add **config.dict() at PeftModel.from_pretrained(model, model_desc.peft_model_id_or_path) and DeltaTunerModel.from_pretrained(model, model_desc.peft_model_id_or_path) in both transformer_predictor.py and deepspeed_predictor.py.

IIUC, maybe we should add like L34 instead of dict() right?

@KepingYan
Copy link
Contributor

IIUC, maybe we should add like L34 instead of dict() right?

OK. Thanks.

@Deegue
Copy link
Contributor Author

Deegue commented Jan 17, 2024

Gentle ping @KepingYan for another review since all CI passed.
Will remove models "starcoder" and "llama-2-7b-chat-hf" before it is ready to merge.

bot_id: ''
stop_words: []
config:
use_auth_token: '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'
Copy link
Contributor

Choose a reason for hiding this comment

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

use_auth_token cannot be written directly in config yaml, it needs to be set in CI file. @jiafuzha please help confirm this.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, strictly speaking. we cannot. But it's only read-only key. If it can pass github security check, I think we can leave it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the environment of our CI nodes have env.HF_ACCESS_TOKEN configured. I think I can try to get use_auth_token runtime from env instead of passing in yaml directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's better.

Maybe we can have a unit test later to verify if use_auth_token is passed correctly. This ticket exposed and fixed the token bug in several places, which shows the right value of CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed them from CI. Let's merge this first and I will create a follow-up PR to hide auth token.

@Deegue Deegue changed the title [Inference] Add model starcoder and enable llama2 [Inference] Fix auth token and add models starcoder and llama2 Jan 17, 2024
@Deegue
Copy link
Contributor Author

Deegue commented Jan 19, 2024

All tests passed, auth token is removed from yaml. Removed starcoder and llama2 from CI. Ping @KepingYan @jiafuzha for review, thanks!

@jiafuzha
Copy link
Contributor

All tests passed, auth token is removed from yaml. Removed starcoder and llama2 from CI. Ping @KepingYan @jiafuzha for review, thanks!

how is auth_token passed to CI? huggingface-cli login?

@Deegue
Copy link
Contributor Author

Deegue commented Jan 19, 2024

All tests passed, auth token is removed from yaml. Removed starcoder and llama2 from CI. Ping @KepingYan @jiafuzha for review, thanks!

how is auth_token passed to CI? huggingface-cli login?

I used the environment variables env.HF_ACCESS_TOKEN to get and rewrite the yaml, to pass the auto token.

CMD=$(cat << EOF
import yaml
if ("${{ matrix.model }}" == "starcoder"):
conf_path = "inference/models/starcoder.yaml"
with open(conf_path, encoding="utf-8") as reader:
result = yaml.load(reader, Loader=yaml.FullLoader)
result['model_description']["config"]["use_auth_token"] = "${{ env.HF_ACCESS_TOKEN }}"
with open(conf_path, 'w') as output:
yaml.dump(result, output, sort_keys=False)
if ("${{ matrix.model }}" == "llama-2-7b-chat-hf"):
conf_path = "inference/models/llama-2-7b-chat-hf.yaml"
with open(conf_path, encoding="utf-8") as reader:
result = yaml.load(reader, Loader=yaml.FullLoader)
result['model_description']["config"]["use_auth_token"] = "${{ env.HF_ACCESS_TOKEN }}"
with open(conf_path, 'w') as output:
yaml.dump(result, output, sort_keys=False)
EOF
)
docker exec "${TARGET}" python -c "$CMD"

@Deegue
Copy link
Contributor Author

Deegue commented Jan 23, 2024

Hi @KepingYan @jiafuzha , could you take a second look whether there are further comments?

@KepingYan
Copy link
Contributor

image
Environment variable doesn’t seem to take effect, please add this step and try again.

- name: Load environment variables
run: cat /root/actions-runner-config/.env >> $GITHUB_ENV

@Deegue
Copy link
Contributor Author

Deegue commented Jan 23, 2024

image Environment variable doesn’t seem to take effect, please add this step and try again.

- name: Load environment variables
run: cat /root/actions-runner-config/.env >> $GITHUB_ENV

Thanks @KepingYan . It seems no env file on our CI nodes.

@Deegue
Copy link
Contributor Author

Deegue commented Feb 5, 2024

All test passed, removed starcoder and llama-2-7b-chat-hf from CI. Ping @KepingYan for review.

Copy link
Contributor

@KepingYan KepingYan left a comment

Choose a reason for hiding this comment

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

LGTM

@Deegue Deegue merged commit 6d72097 into intel:main Feb 7, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants