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.

Sign up for GitHub

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

Jump to bottom

[SambaNova] Integrate SambaNova Systems to oumi inference #1415

Merged
merged 9 commits into from
Feb 11, 2025
Merged

[SambaNova] Integrate SambaNova Systems to oumi inference #1415

merged 9 commits into from
Feb 11, 2025

Conversation

ctseng777
Copy link
Contributor

@ctseng777 ctseng777 commented Feb 8, 2025 •
edited
Loading

Description

Related issues

Fixes # (#1417)

Before submitting

  • This PR only changes documentation. (You can ignore the following checks in that case)
  • Did you read the contributor guideline Pull Request guidelines?
  • Did you link the issue(s) related to this PR in the section above?
  • Did you add / update tests where needed?

Reviewers

At least one review from a member of oumi-ai/oumi-staff is required.

@ctseng777 ctseng777 mentioned this pull request Feb 9, 2025
Closed
@ctseng777
Corrected authroization header. Added e2e test. Able to run pass the …
fce0c25 
…e2e test. Still need to revise notebook tutorial

Revise the location of configuration file

Precommit auto fix

pre-commit auto fixes

Add SAMBANOVA literal
@xrdaukar xrdaukar requested review from taenin and oelachqar February 9, 2025 18:42
@xrdaukar
Copy link
Collaborator

xrdaukar commented Feb 9, 2025

@ctseng777 Thank you for the contribution!

@ctseng777
Add test yaml file. Move the sambanova_infer_test to e2e. Add pytest …
5d28927 
…skip if the API key is not present

Move the sambanova_infer_test to e2e. Add pytest skip if the API key is not present

precommit auto fix
@ctseng777 ctseng777 marked this pull request as ready for review February 10, 2025 03:09
@ctseng777 ctseng777 requested a review from xrdaukar February 10, 2025 03:09
@ctseng777 ctseng777 mentioned this pull request Feb 10, 2025
Closed
5 tasks
taenin
taenin approved these changes Feb 10, 2025
View reviewed changes
Copy link
Collaborator

@taenin taenin 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 contribution! +1 to @xrdaukar 's comments above. Otherwise this looks great!

taenin
taenin requested changes Feb 10, 2025
View reviewed changes
Copy link
Collaborator

@taenin taenin left a comment

Choose a reason for hiding this comment

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

One more fix is required for get_supported_params

tests/unit/inference/test_sambanova_inference_engine.py Show resolved Hide resolved
@ctseng777 ctseng777 requested review from taenin and xrdaukar February 11, 2025 06:24
@ctseng777
Copy link
Contributor Author

I've addressed all comments. Let me know the procedure of merging to original fork main. Or will someone merge for me?

@xrdaukar
Copy link
Collaborator

I've addressed all comments. Let me know the procedure of merging to original fork main. Or will someone merge for me?

We'll merge for you!

@xrdaukar xrdaukar merged commit 20faca9 into oumi-ai:main Feb 11, 2025
3 checks passed
@ciaralema
Copy link
Collaborator

Hey @ctseng777 - thank you for doing this! We'd love to chat to get your quick feedback - would you mind pinging me in Discord? My handle is @ ciara_oumi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taenin taenin taenin approved these changes

@xrdaukar xrdaukar xrdaukar approved these changes

@oelachqar oelachqar Awaiting requested review from oelachqar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ctseng777 @xrdaukar @ciaralema @taenin