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

test: OpenAI frontend invalid chat tokenizer network issue WAR #7779

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

kthui
Copy link
Contributor

@kthui kthui commented Nov 8, 2024

What does the PR do?

Allow test_chat_completions_invalid_chat_tokenizer test to be skipped if there is a network issue when accessing the invalid tokenizer.

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

N/A

Where should the reviewer start?

N/A

Test plan:

The affected test should continue to pass on existing pipeline, and skipped when there is a network issue.

  • CI Pipeline ID: N/A

Caveats:

N/A

Background

N/A

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

N/A

@kthui kthui added the PR: test Adding missing tests or correcting existing test label Nov 8, 2024
@kthui kthui force-pushed the jacky-openai-invalid-chat-tokenizer-network-issue branch from ae9a260 to 039b510 Compare November 8, 2024 18:55
@kthui kthui force-pushed the jacky-openai-invalid-chat-tokenizer-network-issue branch from 3a79a55 to 35ce6dd Compare November 8, 2024 19:20
@kthui
Copy link
Contributor Author

kthui commented Nov 8, 2024

When there is a network issue:

tests/test_chat_completions.py::TestChatCompletionsTokenizers::test_chat_completions_invalid_chat_tokenizer
...
transformers.__version__='4.45.1'
SKIPPED (HuggingFace network issues)

@kthui kthui requested a review from rmccorm4 November 8, 2024 19:37
@kthui kthui marked this pull request as ready for review November 8, 2024 19:37
tokenizer=invalid_chat_tokenizer, server=server, backend=backend
)
except OSError as e:
expected_msg = f"We couldn't connect to 'https://huggingface.co' to load this file, couldn't find it in the cached files and it looks like {invalid_chat_tokenizer} is not the path to a directory containing a file named config.json."
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now, but just be wary if the message changes slightly in the future it might not be an exact match.

Copy link
Contributor Author

@kthui kthui Nov 8, 2024

Choose a reason for hiding this comment

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

Yes. I want to be cautious on this first, to avoid skipping the test for anything else.

If the message varies in the future, I think we can update the condition to detect if the network communication is disabled, i.e. HF_HUB_OFFLINE, and skip the test based on that.

Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix Jacky!

For future reference, this error is probably due to some env var or change to transformers build to disable network communication with HF and only allow pre-downloaded local files, such as via HF_HUB_OFFLINE

@kthui kthui merged commit 73d22e2 into main Nov 8, 2024
3 checks passed
@kthui kthui deleted the jacky-openai-invalid-chat-tokenizer-network-issue branch November 8, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: test Adding missing tests or correcting existing test
Development

Successfully merging this pull request may close these issues.

2 participants