Skip to content
This repository was archived by the owner on Apr 23, 2026. It is now read-only.

fix: Reverting previous change to test job - #3065

Merged
nathan-weinberg merged 1 commit into
instructlab:mainfrom
dmartinol:inject_hf_token
Jan 30, 2025
Merged

fix: Reverting previous change to test job#3065
nathan-weinberg merged 1 commit into
instructlab:mainfrom
dmartinol:inject_hf_token

Conversation

@dmartinol

@dmartinol dmartinol commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

Removed injection of HF token.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Functional tests have been added, if necessary.
  • E2E Workflow tests have been added, if necessary.

@dmartinol
reverting previous change
1480816 
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
@dmartinol
dmartinol marked this pull request as ready for review January 30, 2025 18:22
@mergify mergify Bot added the CI/CD Affects CI/CD configuration label Jan 30, 2025
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jan 30, 2025
@nathan-weinberg

nathan-weinberg commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

Big credit to @bbrowning for realizing this is not the approach we want to take: #3054 (comment)

bbrowning
bbrowning approved these changes Jan 30, 2025
View reviewed changes

@bbrowning bbrowning left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix! If you need a test job that runs against PRs that does require a valid HuggingFace token, we can discuss separately how to do that as it would need to happen in a different test suite run from a different workflow.

@mergify mergify Bot added ci-failure PR has at least one CI failure and removed one-approval PR has one approval from a maintainer labels Jan 30, 2025
@nathan-weinberg

nathan-weinberg commented Jan 30, 2025

Copy link
Copy Markdown
Contributor

Manually merging since this is a revert

@nathan-weinberg
nathan-weinberg merged commit fb2e7be into instructlab:main Jan 30, 2025
@dmartinol

dmartinol commented Jan 30, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for the quick fix! If you need a test job that runs against PRs that does require a valid HuggingFace token, we can discuss separately how to do that as it would need to happen in a different test suite run from a different workflow.

@anastasds do we want to insist with this functional test? wait for quantized and public embedding model instead? other options?

@dmartinol
dmartinol deleted the inject_hf_token branch January 30, 2025 20:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@bbrowning bbrowning bbrowning approved these changes
@nathan-weinberg nathan-weinberg nathan-weinberg approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CI/CD Affects CI/CD configuration ci-failure PR has at least one CI failure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dmartinol @nathan-weinberg @bbrowning