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

add alibabacloud ai search notebook #324

Merged

Conversation

Huaixinww
Copy link
Contributor

Related to https://www.elastic.co/guide/en/elasticsearch/reference/master/infer-service-alibabacloud-ai-search.html
This PR adds a Jupyter notebook that contains an end-to-end example of using the Inference API with the AlibabaCloud AI Search service.

Copy link

gitnotebooks bot commented Sep 3, 2024

Found 1 changed notebook. Review the changes at https://app.gitnotebooks.com/elastic/elasticsearch-labs/pull/324

@serenachou
Copy link
Collaborator

@elastic/search-experiences-team please review

@@ -0,0 +1,492 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Commented on notebook notebooks/alibabacloud-ai-search/inference-alibabacloud-ai-search.ipynb Cell 2 Line 5

# Requirements

For this example, you will need:

- An Elastic deployment with minimum **4GB machine learning node**

We use Alibaba's Cloud right? So we dont need a ML Node here as the work is proxied through to Alibaba

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion! I have removed the restriction on the 4GB machine learning node.

"# Create the client instance\n",
"client = Elasticsearch(\n",
" # For local development\n",
" # hosts=[\"http://localhost:9200\"]\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also use getpass for storing the hosts variable like this if you'd like:

hosts=getpass.getpass("Host: ")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I'v used getpass to store the hosts for improving security.

Copy link

cla-checker-service bot commented Sep 18, 2024

💚 CLA has been signed

@Huaixinww Huaixinww force-pushed the feature/add-alibabacloud-ai-search-notebook branch from fc4dd2d to 1b1ac85 Compare September 18, 2024 03:32
@@ -0,0 +1,483 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Commented on notebook notebooks/alibabacloud-ai-search/inference-alibabacloud-ai-search.ipynb Cell 2 Line 7

# Requirements

For this example, you will need:

- An Elastic deployment:
  - we'll be using [Alibaba Elasticsearch](https://www.aliyun.com/product/bigdata/elasticsearch) for this example.
- Elasticsearch 8.16 or above

is this correct as 8.16 has not been released yet. SHould this be 8.15 or is this a notebook that should be published on release of 8.16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This notebook is related to the following pull request: elastic/elasticsearch#111181, which is labeled as version 8.16.0. The inference API in version 8.15 does not support the Alibaba Cloud AI Search Model, so I believe this notebook should be published with the release of version 8.16.

Copy link
Member

Choose a reason for hiding this comment

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

OK great. Approving for now but will add a note that this is on hold to merge until 8.16 release cc @JessicaGarson .

@joemcelroy
Copy link
Member

Ah you also need to run pre-commit, to format the notebook https://github.com/elastic/elasticsearch-labs/blob/main/CONTRIBUTING.md#pre-commit-hook

@Huaixinww
Copy link
Contributor Author

Ah you also need to run pre-commit, to format the notebook https://github.com/elastic/elasticsearch-labs/blob/main/CONTRIBUTING.md#pre-commit-hook

Thank you for the reminder! I've already run the pre-commit hook to format the notebook. Let me know if you need anything else!

@JessicaGarson
Copy link
Contributor

This notebook isn't passing some of the checks but I'm happy to merge this after. I've found this to be helpful in the past when some of my code was in a similar state:

python -m venv .venv
.venv/bin/pip install -qqq -r requirements-dev.txt
.venv/bin/pre-commit install

@Huaixinww feel free to ping me if you need any help here.

@Huaixinww
Copy link
Contributor Author

Huaixinww commented Nov 20, 2024

This notebook isn't passing some of the checks but I'm happy to merge this after. I've found this to be helpful in the past when some of my code was in a similar state:

python -m venv .venv
.venv/bin/pip install -qqq -r requirements-dev.txt
.venv/bin/pre-commit install

@Huaixinww feel free to ping me if you need any help here.

Hi @JessicaGarson, thanks a lot for ur suggestion! I've already run the pre-commit, the pre-commit check is successful. This notebook isn't passing some of the checks because I didn't set the ELASTIC_USER and ELASTIC_PASSWORD in my demo, I want to know how to pass this check? should I add fake ELASTIC_USER and ELASTIC_PASSWORD?

@joemcelroy
Copy link
Member

because this notebook relies on an alibaba account, we cannot rely on our automated tests to validate this notebook from regressions. This notebook needs to be excluded in https://github.com/elastic/elasticsearch-labs/blob/main/bin/find-notebooks-to-test.sh#L3

@JessicaGarson worth adding this in the contribute guide for when contributing notebooks to notebooks folder.

@joemcelroy
Copy link
Member

pushed up to exclude it from the CI test script and also moved the notebook into integrations.

@joemcelroy joemcelroy merged commit 6225919 into elastic:main Nov 20, 2024
5 checks passed
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.

4 participants