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

add n to the acomplete and refactor error handler #170

Closed
wants to merge 8 commits into from
Closed

add n to the acomplete and refactor error handler #170

wants to merge 8 commits into from

Conversation

zhaochenyang20
Copy link
Contributor

Description

  1. I create ERROR_ERRORS_TO_MESSAGES to make the error handling clearer for _throttled_openai_completion_acreate and _throttled_openai_chat_completion_acreate.
  2. There is no TimeoutError in openai.error, so I delete it.
  3. I add a new parameter n for both completion, which is the number of responses to generate for each API call.

References

Blocked by

  • I find that the main branch now can't pass the pre-commit due to other files. 🤔

@zhaochenyang20
Copy link
Contributor Author

I used pre-commit run --show-diff-on-failure --color=always --all-files to run pre-commit. But it changed 21 other files. Possibly due to different settings of pre-commit. 🤔

@cabreraalex
Copy link
Member

Hmm yeah looks like the isort settings are different?

@zhaochenyang20
Copy link
Contributor Author

Hmmm. I checked the isort setting. But I didn't find problem. I will uninstall isort and reinstall it tonight to have a try.

@zhaochenyang20
Copy link
Contributor Author

Hey Alex, I tried loads of times and I feel that our configuration for pre-commit is merely the same. This is mine:

repos:
  - repo: https://github.com/python/black.git
    rev: 22.3.0
    hooks:
      - id: black
        files: '\.py$'
  - repo: https://github.com/PyCQA/flake8
    rev: 5.0.4
    hooks:
      - id: flake8
        name: flake8
        additional_dependencies:
          - flake8-absolute-import
          - flake8-black>=0.1.1
          - flake8-pep585>=0.1.6
        entry: flake8
        files: '\.py$'
      - id: flake8
        name: docstring
        additional_dependencies:
          - flake8-docstrings>=1.6
        args:
          - --docstring-convention=google
          - --select=D
        entry: flake8
        files: '\.py$'
      - id: flake8
        name: future-import
        additional_dependencies:
          - flake8-future-import
        args:
          - --select=FI
        entry: flake8
        files: '\.py$'
  - repo: https://github.com/pycqa/isort.git
    rev: 5.12.0
    hooks:
      - id: isort
        args: ["--profile", "black"]
        files: '\.py$'
  - repo: https://github.com/sondrelg/pep585-upgrade
    rev: v1.0.1
    hooks:
      - id: upgrade-type-hints
        files: '\.py$'
  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: 'v0.981'
    hooks:
      - id: mypy
        additional_dependencies:
          - types-requests
        files: '\.py$'
  - repo: https://github.com/DavidAnson/markdownlint-cli2
    rev: v0.5.1
    hooks:
    - id: markdownlint-cli2
    - id: markdownlint-cli2-fix
···

@zhaochenyang20
Copy link
Contributor Author

zhaochenyang20 commented Jul 22, 2023

Would you please clone my branch and run pre-commit on your device?

I've tried this on my laptop, on TIR, and searched Google and StackOverflow. All failed. 😭

@neubig
Copy link
Collaborator

neubig commented Jul 22, 2023

Thanks @zhaochenyang20 ! I did it and it worked. Could you please select "allow edits from maintainers" for this pull request? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@zhaochenyang20
Copy link
Contributor Author

Sorry. I run all the processes again. Now it also works on my device. 😂

@zhaochenyang20
Copy link
Contributor Author

I select it!

@zhaochenyang20
Copy link
Contributor Author

Sorry that I close this PR and delete the previous fork. I created a new PR: #171

@neubig Thanks so much!

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.

4 participants