-
Notifications
You must be signed in to change notification settings - Fork 263
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
Removing quotes from udf_metadata_key #1026
Removing quotes from udf_metadata_key #1026
Conversation
modified: evadb/parser/evadb.lark Removed .value from key_value_pair[0] post the change in type modified: evadb/parser/lark_visitor/_functions.py Replaced string key to ID_LITERAL in test query modified: test/unit_tests/parser/test_parser.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.
👋 Hello @hershd23, thanks for submitting a EVA DB PR 🙏 To allow your work to be integrated as seamlessly as possible, we advise you to:
- ✅ Verify that your PR is up-to-date with
georgia-tech-db/eva
master
branch. If your PR is behind you can update your code by clicking the 'Update branch' button or by runninggit pull
andgit merge master
locally. - ✅ Verify that all EVA DB Continuous Integration (CI) checks are passing.
- ✅ Reduce changes to the absolute minimum required for your bug fix or feature addition.
modified: evadb/parser/evadb.lark
modified: test/unit_tests/parser/test_parser.py
Once tests and all work, will go ahead and change the docs as well |
modified: README.md modified: docs/source/reference/evaql/create.rst modified: docs/source/reference/udfs/model-train.rst Replacing udf_metadata_key in Ludwig test modified: test/integration_tests/long/test_model_train.py
Ran the integration test for training the model. It parsed the query correctly and invoked training but gave the following error. Creating an issue for this
|
Thanks for creating an issue! Do you run the experiments on ada-00 or ada-01? |
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.
PR itself looks good. Thanks!
Actually I ran it on my personal machine which has a GPU card |
We can merge this PR. We can investigate the GPU issue in #1028. Thanks! |
Come into my mind that you may also need to update the hugging face functions. |
Oh okay @xzdandy. Will check for that today |
18bd15c
to
0011642
Compare
modified: benchmark/text_summarization/text_summarization_with_evadb.py modified: docs/source/benchmarks/text_summarization.rst modified: docs/source/overview/concepts.rst modified: docs/source/reference/ai/hf.rst modified: docs/source/reference/ai/openai.rst modified: docs/source/reference/ai/yolo.rst modified: docs/source/usecases/object-detection.rst modified: docs/source/usecases/question-answering.rst modified: docs/source/usecases/text-summarization.rst modified: evadb/parser/utils.py modified: evadb/udfs/udf_bootstrap_queries.py modified: test/benchmark_tests/test_benchmark_pytorch.py modified: test/integration_tests/long/interfaces/relational/test_relational_api.py modified: test/integration_tests/long/test_error_handling_with_ray.py modified: test/integration_tests/long/test_huggingface_udfs.py modified: test/integration_tests/long/test_reuse.py
modified: docs/source/benchmarks/text_summarization.rst
@xzdandy @gaurav274 can you take a look at the changes, have incorporated the changes for HuggingFace UDFs as well |
I also see some change changes in the docs causing the docs CI to fail. Should I try to fix them (best effort) in the PR? |
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.
Thanks Hersh for the contribution! Doc failures will be fixed in #1035.
Verified on Python 3.10
- All notebook testcases passed
- test/integration_tests/long/test_model_train.py passed when running as an independent test
- 4 testcases failed when
bash script/test/test.sh -m "LONG INTEGRATION"
FAILED test/integration_tests/long/test_model_train.py::ModelTrainTests::test_ludwig_automl - evadb.executor.executor_utils.ExecutorError: No best trial found. Please check if you specified the correct default metric (metric_...
FAILED test/integration_tests/long/test_udf_executor.py::UDFExecutorTest::test_should_create_udf_with_metadata - lark.exceptions.UnexpectedToken: Unexpected token Token('STRING_LITERAL', "'CACHE'") at line 6, column 19.
FAILED test/integration_tests/long/test_udf_executor.py::UDFExecutorTest::test_should_return_empty_metadata_list_if_udf_is_removed - lark.exceptions.UnexpectedToken: Unexpected token Token('STRING_LITERAL', "'CACHE'") at line 6, column 19.
FAILED test/integration_tests/long/interfaces/relational/test_relational_api.py::RelationalAPI::test_create_udf_with_relational_api - AssertionError: "CREA[50 chars]Face 'task' 'automatic-speech-recognition' 'mo[22 chars]ase'" != "CREA[50 chars]Face TASK 'automatic...
A quick check at the error message indicates that test/integration_tests/long/test_model_train.py::ModelTrainTests::test_ludwig_automl
failed because of GPU memory from other testcases are not freed. If the error is not relevant to this PR's change, welcome to create an issue, and we will investigate separately!
Minors:
- remove
.lock_preprocessing
from the pr.
Update:
- test/integration_tests/long/test_model_train.py failed when running with other long integration test cases #1036 created for
test/integration_tests/long/test_model_train.py
deleted: .lock_preprocessing Converted CACHE and BATCH from string literals to UIDs modified: test/integration_tests/long/test_udf_executor.py
Hi @xzdandy have made the following changes
For the other two issues
We have made all udf_metadata_keys into UIDs. Once parsed these are processed and ultimately stored as lowercase strings in the metadata_map. What this test is trying to do is trying to convert the query and args from the API to the QUERY itself. To solve this I need to find the query builder for this which converts the API constructs to an SQL type query. I guess @gaurav274 mentioned that there are some usecases where python API data is converted to query and then run to leverage the optimizations built on the query. Can you point me to the query builder code. I'll try and correct this there. UPDATE : Figured it out |
direct string matching test cases modified: evadb/parser/create_udf_statement.py
@xzdandy made the changes can you take a look |
Are we good to go with this? Can we merge or waiting for @gaurav274 's review. |
I am fixing all merge conflicts now. |
Done done, I have fixed merge conflicts |
One minute to update the forecasting feature to use the new UDF syntax |
Thanks @xzdandy for all your help! |
Replacing udf_metadata_key from string_literal to ID_LITERAL ` modified: evadb/parser/evadb.lark` Removed .value from key_value_pair[0] post the change in type ` modified: evadb/parser/lark_visitor/_functions.py` Replaced string key to ID_LITERAL in test query ` modified: test/unit_tests/parser/test_parser.py` Solves #1010 --------- Co-authored-by: xzdandy <xzdandy@gmail.com>
Replacing udf_metadata_key from string_literal to ID_LITERAL
modified: evadb/parser/evadb.lark
Removed .value from key_value_pair[0] post the change in type
modified: evadb/parser/lark_visitor/_functions.py
Replaced string key to ID_LITERAL in test query
modified: test/unit_tests/parser/test_parser.py
Solves #1010