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

new products dataset release #159

Merged
merged 10 commits into from Dec 29, 2020
Merged

new products dataset release #159

merged 10 commits into from Dec 29, 2020

Conversation

pierreaws
Copy link
Contributor

Issue #, if available:

Description of changes:

new products dataset from MLSL Demo Team. The images bucket link is temporary

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

stage.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@alexchirayath alexchirayath left a comment

Choose a reason for hiding this comment

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

Just a comment pending on @james-jory to provide us an s3 bucket to add the image. The rest of the PR looks good.
Please request a re-review once the bucket is updated in stage.sh

@alexchirayath alexchirayath mentioned this pull request Dec 18, 2020
Copy link
Collaborator

@alexchirayath alexchirayath left a comment

Choose a reason for hiding this comment

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

  1. The sizes of the screenshots added is too large (You can see in your commit, some images have increased by 400%) which lead to issues with CodeCommit upload. Please resize the images to be the same size(if not smaller)

  2. The instructions near emulate shopper profile need to be updated to reflect the new UX. Could you please make changes to it?

@alexchirayath
Copy link
Collaborator

The changes now look got to me. Thanks @pierreaws
pending on @james-jory to provide us an s3 bucket to add the image to update the stage.sh . The rest of the PR looks good.
Please request a re-review once the bucket is updated in stage.sh

@@ -1668,7 +1668,7 @@
"metadata": {},
"outputs": [],
"source": [
"product_id = 22\n",
"product_id = '020a5afe-fb13-4499-a1fa-8594d326eaa0'\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

FUTURE: I think we should change this to a randomly selected product from the items dataframe to make this code more future proof should the products.yaml file change again.

Copy link
Contributor

@james-jory james-jory left a comment

Choose a reason for hiding this comment

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

Pending the bucket change and verifying that staging and deployment works from this bucket as expected, all looks good to me. Let me know when that has been verified. Thank you!

stage.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@alexchirayath alexchirayath left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes @pierreaws

Copy link
Contributor

@james-jory james-jory left a comment

Choose a reason for hiding this comment

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

The Search workshop was timing out indexing the new larger catalog in one call so had to add logic to break products into batches for bulk indexing. Works fine now.

@james-jory james-jory merged commit 817ce73 into aws-samples:master Dec 29, 2020
@alexchirayath
Copy link
Collaborator

The Search workshop was timing out indexing the new larger catalog in one call so had to add logic to break products into batches for bulk indexing. Works fine now.

Ah interesting. Thanks for making adding the bulk index logic !

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.

None yet

3 participants