-
Notifications
You must be signed in to change notification settings - Fork 5
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 to validate the class name-algomo-dev crashloop on 363-char clas… #166
base: main
Are you sure you want to change the base?
Conversation
} | ||
client.schema.create_class(class_obj) | ||
response = client.schema.get(); | ||
print(response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this print statement can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge. |
client.schema.delete_all() | ||
class_obj = { | ||
"vectorizer": "none", | ||
"vectorIndexConfig": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it is not needed when vectorizer is turned off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The Default vectorizer is 'none' in docker file. I can remove this line.
assert (response.status_code == 200) | ||
return response; | ||
|
||
invalid_long_classname(client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
script could be extended to validate property name as well.
property names are used to create directory names, so they may turn up being too long.
echo "Starting Weaviate with auto-schema enabled..." | ||
docker-compose -f apps/create-class-validation/docker-compose.yml up -d | ||
|
||
docker run --network host -t create-class-validation python3 invalid_class_long_name.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no wait_weaviate
in between?
fi | ||
|
||
echo "Starting Weaviate with auto-schema enabled..." | ||
docker-compose -f apps/create-class-validation/docker-compose.yml up -d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO instead of creating new docker compose to change one env variable, existing docker compose could be reused and updated to support new variable apps/weaviate-no-restart-on-crash/docker-compose.yml
.
Default value for variable can be set in apps/weaviate-no-restart-on-crash/.env
and overwritten when needed directly in command:
AUTOSCHEMA_ENABLED=false docker-compose -f apps/weaviate-no-restart-on-crash/docker-compose.yml up -d
This PR has test for invalid class name creation. There are 2 tests to validate the length of character in the name of a class. One test when 'AUTO-SCHEMA-ENABLED' is true and the other when the 'AUTO-SCHEMA-ENABLED' is false.
Please note: I will have to revist this test when the fix is in place. For now, I am asserting if the status code of the class creation is 200.