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/add multimodal audio test #382

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

mdimado
Copy link

@mdimado mdimado commented Jan 22, 2025

Purpose

  • add test for multimodal audio support

Proposed Changes

  • added a new GetAudioToolInput class
  • created a new GetAudioTool class
  • modified the test_multimodal_messages function
  • added audio testing scenario

Issues

Copy link
Contributor

@rachwalk rachwalk left a comment

Choose a reason for hiding this comment

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

Hello, thank you for working on this.

This PR suffers from the same issue as #380
The failing test provides the following output:

 =================================== FAILURES ===================================
_______________________ test_to_langchain_ai_multimodal ________________________

    def test_to_langchain_ai_multimodal():
        payload = HRIPayload(text="Response", images=["img"], audios=["audio"])
        message = HRIMessage(payload=payload, message_author="ai")
    
>       with pytest.raises(
            ValueError
        ):  # NOTE: update when https://github.com/RobotecAI/rai/issues/370 is resolved
E       Failed: DID NOT RAISE <class 'ValueError'>

This one also has a pre-commit formatting issue. You can reproduce these errors on your local machine by running:
pytest -m "not billable" - to reproduce the test output - Note: running just pytestwill trigger all tests,pre-commit run --all- to see the formatting issues detected by the pre-commit. You can also runpre-commit installin your local rai directory, which will automatically runpre-commit` on all the files changed before you make a commit.

I won't be able to merge the PR unless all existing tests pass and the formatting kept up. Hope the feedback I provided helps you out.

@@ -56,6 +54,19 @@ def __init__(
for image in self.images
]
_content.extend(_image_content)

# aduio content handling (used audio/wav as MIME type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# aduio content handling (used audio/wav as MIME type)
# audio content handling (used audio/wav as MIME type)

Copy link
Author

Choose a reason for hiding this comment

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

mb, I'll change that

Comment on lines +58 to +63
def _run(self, name: str):
# simple audio signal (1 second of 440Hz sine wave)
sample_rate = 44100
duration = 1.0
t = np.linspace(0, duration, int(sample_rate * duration))
audio_signal = np.sin(2 * np.pi * 440 * t)
Copy link
Contributor

Choose a reason for hiding this comment

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

A better solution would be to add an actual test.wav file, instead of mocking the input to the too like so.

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.

2 participants