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

Temp fix #51

Merged
merged 2 commits into from
Mar 6, 2024
Merged

Temp fix #51

merged 2 commits into from
Mar 6, 2024

Conversation

lkuligin
Copy link
Collaborator

@lkuligin lkuligin commented Mar 6, 2024

fixed #49

@lkuligin lkuligin merged commit 6702bed into main Mar 6, 2024
12 checks passed
@lkuligin lkuligin deleted the temp_fix branch March 6, 2024 19:09
@eycjur
Copy link

eycjur commented Mar 7, 2024

@lkuligin
Thank you for the correction. But i think this fix is ​​not enough. This is because assertCountEqual does not determine the contents of nested dictionaries (I don't know if this is normal behavior for assertCountEqual, but at least it behaves like that)

For example, the following test will pass

from unittest import TestCase

def test_assert_count_equal():
	TestCase().assertCountEqual(
		{
			"stream": False,
			"safety_settings": None,
			"generation_config": {
				"max_output_tokens": 1,
				"temperature": 0,
				"top_k": 10,
				"top_p": 0.5,
				"stop_sequences": None,
			},
		},
		{
			"stream": False,
			"safety_settings": None,
			"generation_config": {
			},
		}
	)

And in the case of this pull request, I think it is necessary to modify the line below.

libs/vertexai/langchain_google_vertexai/llms.py
            default_value = default_params.get(param_name)
-             if param_value or default_value:
+             if param_value is not None or default_value is not None:
                updated_params[param_name] = (
                    param_value if param_value is not None else default_value
                )
        return updated_params

Thanks!

@lkuligin
Copy link
Collaborator Author

lkuligin commented Mar 7, 2024

@eycjur it's embarassing for me, but you're right! thanks a lot for spotting it. Fixed.

P.S. Please, feel free to contribute yourself next time, we welcome contributions very much!

@eycjur
Copy link

eycjur commented Mar 8, 2024

@lkuligin
I've learned a lot from your code, so I'm glad I could help you out!
Your comments are very encouraging. Please let me contribute next time!

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.

Temperature parameter is ignored when temperature=0
3 participants