Skip to content

fix: Reference segmentation limit #2487

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

Merged
shaohuzhang1 merged 1 commit into from
Mar 4, 2025
Merged

fix: Reference segmentation limit #2487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/application/serializers/application_serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def valid_model_params_setting(model_id, model_params_setting):


class DatasetSettingSerializer(serializers.Serializer):
top_n = serializers.FloatField(required=True, max_value=100, min_value=1,
top_n = serializers.FloatField(required=True, max_value=10000, min_value=1,
error_messages=ErrMessage.float(_("Reference segment number")))
similarity = serializers.FloatField(required=True, max_value=1, min_value=0,
error_messages=ErrMessage.float(_("Acquaintance")))
Expand Down Expand Up @@ -581,7 +581,7 @@ class HitTest(serializers.Serializer):
id = serializers.CharField(required=True, error_messages=ErrMessage.uuid(_("Application ID")))
user_id = serializers.UUIDField(required=False, error_messages=ErrMessage.uuid(_("User ID")))
query_text = serializers.CharField(required=True, error_messages=ErrMessage.char(_("Query text")))
top_number = serializers.IntegerField(required=True, max_value=100, min_value=1,
top_number = serializers.IntegerField(required=True, max_value=10000, min_value=1,
error_messages=ErrMessage.integer(_("topN")))
similarity = serializers.FloatField(required=True, max_value=2, min_value=0,
error_messages=ErrMessage.float(_("Relevance")))
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Mar 4, 2025

Choose a reason for hiding this comment

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

The code you provided is generally clean and follows best practices for Python serialization with Django REST Framework (DRF). However, there are some minor improvements and suggestions:

Improvements:

  1. Consistent Use of max_value: In the DatasetSettingSerializer, both top_n and similarity have max_value=10000. This is consistent, but if this value might change in future versions, it would be good to make it configurable or an environment variable.

  2. Error Message Translation: The translations for "Number" and "relevance" should match across all fields where they appear consistently. Ensure that _ is used correctly for translation throughout the codebase.

Optimization Suggestions:

  • Avoid Hardcoding Values: Instead of having hardcoded values like required=True in each field configuration, consider using defaults or external settings files if these parameters are subject to change frequently.

  • Use Consistent Field Names: While this isn't necessarily a technical issue, maintaining consistency in field names can improve readability and maintainability.

Summary:

Overall, the code looks mostly correct and functional. By making slight adjustments, such as configuring thresholds more dynamically and ensuring consistency in error message usage, you can enhance the robustness and flexibility of your serializer logic.

Expand Down
2 changes: 1 addition & 1 deletion apps/dataset/serializers/dataset_serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ class HitTest(ApiMixin, serializers.Serializer):
id = serializers.CharField(required=True, error_messages=ErrMessage.char("id"))
user_id = serializers.UUIDField(required=False, error_messages=ErrMessage.char(_('user id')))
query_text = serializers.CharField(required=True, error_messages=ErrMessage.char(_('query text')))
top_number = serializers.IntegerField(required=True, max_value=100, min_value=1,
top_number = serializers.IntegerField(required=True, max_value=10000, min_value=1,
error_messages=ErrMessage.char("top number"))
similarity = serializers.FloatField(required=True, max_value=2, min_value=0,
error_messages=ErrMessage.char(_('similarity')))
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Mar 4, 2025

Choose a reason for hiding this comment

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

There is an issue in the top_number field definition where it currently limits to a maximum value of 100. This might not be sufficient if you expect higher numbers of results from your hits test functionality.

Irregularity or Potential Issue:

The current limit on top_number is set too low (100). It should be increased to allow for handling larger sets of results if needed.

Top number should have a more flexible maximum range, possibly up to 10,000 or even higher depending on expected use cases.

Optimization Suggestion:

Consider raising the upper limit on top_number to 10,000 or another reasonable value that suits your application's needs better than 100. Here’s how you can adjust the field:

class HitTest(ApiMixin, serializers.Serializer):
    id = serializers.CharField(required=True, error_messages=ErrMessage.char("id"))
    user_id = serializers.UUIDField(
        required=False,
        error_messages=ErrMessage.char(_('user id'))
    )
    query_text = serializers.CharField(required=True, error_messages=ErrMessage.char(_('query text')))
    top_number = serializers.IntegerField(
        required=True,
        max_value=10000,  # Increased from 100 to 10000
        min_value=1,
        error_messages=ErrMessage.char('top number')
    )
    similarity = serializers.FloatField(
        required=True,
        max_value=2,
        min_value=0,
        error_messages=ErrMessage.char(_('similarity'))
    )

This change will make the API capable of handling requests with a significantly greater number of top-n results, which may be beneficial in scenarios requiring more detailed data retrieval based on similarity scores.

Expand Down