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

AWS Interactive Video Service (IVS) Demo, and Personalised Discount addition to the retail demo #96

Merged
merged 128 commits into from Dec 2, 2020

Conversation

AkramDweikat
Copy link
Contributor

… Summary as follow:

  • IVS: Addition of IVS container, IVS cloud formation.
  • UI: Adding UI Tab and page for the streams, its products, recommendations and discounts.
  • UI: Adding Reset Amazon Personalize button on the user profile.
  • UI: Refactoring for the menu.
  • Amazon Personalize: Addition of new campaign for discounts, update to the container service code.
  • Amazon Personalize: Additional Jupyter notebook, remove dependancy on bucket to stage data, instead generate it from script.
  • Update to the readmes and documentations.
    This work was done by Dae.mn Team.
    Co-authored-by: Akram Dweikat akram.dweikat@gmail.com
    Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
    Co-authored-by: Damien Jade Duff damien.duff@daemonsolutions.com
    Co-authored-by: daemon-joe joe.major@dae.mn

Issue #, if available:

Description of changes:

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

* Add optimizely container params (aws-samples#82)

* added optimizely sdk key param to containers

* logging add

* pprint

* ssm:GetParameters to task exec role

* rename ssm policy for execution role

* removed debug logging

* resource_bucket_path in CopyImagesLambdaFunction (aws-samples#84)

* Bump selfsigned version for node-forge vuln

* remove phone number field from signUp form

* Update the READMEs and instructions to reflect changes in signup

Co-authored-by: Igor Krtolica <1186878+manbearshark@users.noreply.github.com>
Co-authored-by: Bastien <bastil@amazon.co.uk>
Co-authored-by: James Jory <james@jamesjory.com>
…ulnerability

Serialize javascript vulnerability
… Summary as follow:

* IVS: Addition of IVS container, IVS cloud formation.
* UI: Adding UI Tab and page for the streams, its products, recommendations and discounts.
* UI: Adding Reset Amazon Personalize button on the user profile.
* UI: Refactoring for the menu.
* Amazon Personalize: Addition of new campaign for discounts, update to the container service code.
* Amazon Personalize: Additional Jupyter notebook, remove dependancy on bucket to stage data,  instead generate it from script.
* Update to the readmes and documentations.
This work was done by Dae.mn Team.
Co-authored-by: Akram Dweikat <akram.dweikat@gmail.com>
Co-authored-by: Akram Dweikat  <akram.dweikat@dae.mn>
Co-authored-by: Damien Jade Duff <damien.duff@daemonsolutions.com>
Co-authored-by: daemon-joe <joe.major@dae.mn>
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.

This is a massive PR and probably should have been done in phases. But we'll get through it. Excited to see IVS integrated! Some great work here.

General questions:

  • Why has the delete personalize resources CR been deleted? How will Personalize resources be cleaned up when the user deletes the CFN stack?
  • Should the 1.3 personalized discounts notebook be 1.2 and the cleanup notebook renamed to 1.3? The idea of the cleanup notebook is to cleanup after you've finished the other notebook(s) in the folder.
  • The discount notebook seems to be more focused on validating the interaction generation methodology than as an educational tool for customers. For example, all of the steps a customer would normally want to learn on how to use the Personalize service is hidden away in the lambda function. I'm also wondering if the 4th campaign is necessary. It's not completely clear what its purpose is. Could contextual metadata have been used instead with the existing campaigns?

See more detailed questions throughout PR.

Thanks!

Comment on lines 162 to 188
- personalize:ListCampaigns
- personalize:ListDatasetGroups
- personalize:ListSolutions
- personalize:ListSchemas
- personalize:ListSolutionVersions
- personalize:ListDatasetImportJobs
- personalize:ListDatasets
- personalize:ListEventTrackers
- personalize:CreateSchema
- personalize:CreateDatasetGroup
Resource: '*'
- Effect: Allow
Action:
- personalize:DescribeSolutionVersion
- personalize:CreateSolutionVersion
- personalize:CreateDatasetImportJob
- personalize:CreateSolution
- personalize:DescribeDatasetGroup
- personalize:DescribeDatasetImportJob
- personalize:DescribeSolution
- personalize:DescribeEventTracker
- personalize:CreateCampaign
- personalize:CreateDataset
- personalize:CreateEventTracker
- personalize:CreateFilter
Resource:
- !Sub 'arn:aws:personalize:${AWS::Region}:${AWS::AccountId}:*/retaildemostore*'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were these removed? Please scope back down.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 on this comment. Scoping down permissions whenever possible is for security around the application. If you need all permissions or on all resources, please break down into multiple policies which will be scoped accordingly. If there are any * policies, please provide a comment linking to AWS documentation that requires *

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem, will scope these down.

@@ -243,27 +231,37 @@ Resources:
- "PersonalizePreCreateScheduledRule"
- "Arn"

####################### Personalize Resource Delete Custom Resource #######################
# Custom resource to launch IVS create channels function
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment does not match the following resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

Typo - Will amend

Comment on lines 455 to 456
# e.g. if client is pulling recommendations then reranking them for example
# - this should not happen as the first recommendation pull would already be ranked
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we rerank recommendations?

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment was trying to explain a potential corner case but you're absolutely right that it is unlikely anyone would try that - will delete the comment to avoid confusion.

response_items = []
for ranked_item in ranked_items:
item = item_map.get(ranked_item.get('itemId'))
item['old_url'] = item['url']
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this used for?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the existing reranking code, which we endeavoured to keep as intact as possible considering we also repurposed it for discounts, the item URL is retrieved and the experiment ID added. This is presumably so that downstream it can be seen and picked up (though client-side mostly the experiment dictionary is used). We attempted to remove references to the experiment if the product was not involved in the discount recommendation but clearly the better approach overall would be to leave it in. Another approach would be to introduce into the experimentation a separate resolver type to handle with the "picking" type experiment.



@app.route('/reset/realtime', methods=['POST'])
def reset_realtime():
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems out of scope for the IVS integration. What is the requirement here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this demo is going to be running for a number of days at a conference provided functionality to reset the campaigns to their default state in case the output of the campaigns was to drift over the course of a number of demos.

This reset functionality used to be possible with Amazon Personalize by deleting the so-called "EVENT_INTERACTIONS" dataset but this dataset seems deprecated so we experimented with reset options and ultimately needed to:

  • Delete dataset group & all content
  • Recreate dataset group with a new name
  • Recreate solutions and campaigns with new names

Hopefully this provides some background for some of the other changes (e.g. why we merged delete and create functionality into one lambda).

Following yesterday's (22nd Oct) call we are also aware of the blue-green deployment approach that other teams have been working on - we should discuss this further on our next call so we can unify our approach to this sort of functionality.

FFMPEG_SUBS_COMMAND = "ffmpeg -i \"{video_filepath}\" \"{subtitle_path}\""

# -- Default stream details
# These details will be returned by the API if the 'use default IVS streams' option is chosen for deployment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do the playback URLs come from? Who owns and maintains them? Can they change?

Copy link
Contributor

Choose a reason for hiding this comment

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

We were asked to ensure that streams containing realistic content (ie. the vlog-style videos we have purchased) would be available to all users. Due to licensing issues, we are unable to distribute the video files to other users of the demo so we have been asked to deploy persistent streams which all users can consume using the IVS ingest URL. The AWS Retail team will maintain them going forward.

The streams can change if there is a full redeployment of the underlying CloudFormation stack. In this case the default streams would need to be updated here. However, stream URLs would not change on new container deployments (we are using the same CD pipeline as is used in this repo).

The AWS retail team is in the process of obtaining a domain for the IVS demo; we could possibly use that to make the default URLs persistent. One for discussion on a call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to externalize the default stream details in a JSON file or something? Otherwise we're going to have to make changes to source when the product IDs change or when new/different default streams are added. Product IDs will be changing to UUIDs soon. I get that the defaults shouldn't change often but having the hardcoded makes it harder to maintain.

Copy link
Contributor

Choose a reason for hiding this comment

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

We will make this change. Default streams will be picked up from the videos/ folder where user-provided videos may be provided.

src/videos/src/videos-service/app.py Show resolved Hide resolved
@james-jory
Copy link
Contributor

Additional questions:

  • The IVS integration looks to only support pre-recorded videos. How can a live stream be done using the integration?
  • Will there be a workshop notebook added to walk a developer through how to add IVS to the Retail Demo Store and run a live stream?
  • Given the scope of changes, have you done regression testing against the full application?
  • The delivery report (separate doc) indicates that CFN deploys sometimes fail and that it is a pre-existing issue. Please provide details on failures you have experienced so they can be fixed. There are no known/tracked issues with deployment at this time.

Thanks!

CATEGORY_AFFINITY_PROBS = np.array(CATEGORY_AFFINITY_PROBS)


def generate_data():
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation adds a third algorithm/approach for creating interactions in the project.

  • The original interaction generator in the Personalize notebook.
  • The generator framework introduced under generators.
  • This implementation.

The plan is to consolidate and move towards a single algorithm rather than splinter into multiple approaches. Can these improvements be incorporated into the generators algorithm?

Copy link
Contributor

Choose a reason for hiding this comment

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

No doubt it is possible.

The challenge would be the same - to allow for balance across products & categories (if interactions for a particular category are "spread too thin" we can end up with nothing recommended from the category and some items can get forgotten). If more products and categories are in the process of being added to the demo, this may be less of an issue? Note that the old interactions generator also considers gender as a factor which we would need to bring in to the new one (but this was one cause of imbalance).

Alternatively, we could fold any changes in when the original interaction generator is folded in (it seems this is planned?).

style: makeup
description: Perfect for trialling different shades, each palette contains 4 different shades.
price: 145.00
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reasoning behind the item IDs?
I see some really large IDS (1000567891) and some that are close to the existing items id (101,102...)

Copy link
Contributor

Choose a reason for hiding this comment

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

I've replied to a similar question here.

We are happy to standardise for consistency if required

@daemon-joe
Copy link
Contributor

This is a massive PR and probably should have been done in phases. But we'll get through it. Excited to see IVS integrated! Some great work here.

General questions:

  • Why has the delete personalize resources CR been deleted? How will Personalize resources be cleaned up when the user deletes the CFN stack?
  • Should the 1.3 personalized discounts notebook be 1.2 and the cleanup notebook renamed to 1.3? The idea of the cleanup notebook is to cleanup after you've finished the other notebook(s) in the folder.
  • The discount notebook seems to be more focused on validating the interaction generation methodology than as an educational tool for customers. For example, all of the steps a customer would normally want to learn on how to use the Personalize service is hidden away in the lambda function. I'm also wondering if the 4th campaign is necessary. It's not completely clear what its purpose is. Could contextual metadata have been used instead with the existing campaigns?

See more detailed questions throughout PR.

Thanks!

Thanks for the feedback James and for taking the time to discuss with us yesterday. I've replied to the code-specific comments above where appropriate and I'll try to give answers to the more general questions here. I'm sure these will be the basis for some of our discussions going forward as well

  1. In order to implement the "Personalize reset" button we needed to both delete and create Personalize resources so we shifted the delete functionality to the one pre-create-personalize-campaigns.py Lambda (re-using existing code where possible). We hoped that by combining the management of the Personalize infrastructure into a single Custom Resource we could simplify the management of the Personalize resources as well.

  2. Because this notebook does not currently build resources, the order does not matter a lot. If the notebook is expanded to build the discount campaigns, then the order will matter.

3a. We can certainly expand out the notebook and put the interactions generation and Personalize building directly into it. For the latter (building Personalize), the required steps are already tackled in notebook 1.1 so we opted to use the deployed pre-create lambda from this notebook to show how that might be used. For the former (generating interactions) we felt like explaining the additional logic for simulating interactions may be counterproductive from a perspective of teaching Personalize use.

3b. We agree that, though we have not validated it, it could have been used and would have decreased the number of campaigns as suggested and shown off this feature of Personalize as a part of a series of Personalize workshops.
We note that there are a lot of different ways of handling discounts within Personalize and the decision we made was based on a desire to keep the IVS demo standalone alongside with Personalized Discounts.

@daemon-joe
Copy link
Contributor

Additional questions:

  • The IVS integration looks to only support pre-recorded videos. How can a live stream be done using the integration?
  • Will there be a workshop notebook added to walk a developer through how to add IVS to the Retail Demo Store and run a live stream?
  • Given the scope of changes, have you done regression testing against the full application?
  • The delivery report (separate doc) indicates that CFN deploys sometimes fail and that it is a pre-existing issue. Please provide details on failures you have experienced so they can be fixed. There are no known/tracked issues with deployment at this time.

Thanks!

  1. The current integration doesn’t support live streaming, since the Videos service currently returns only the details of the channels created by CloudFormation to stream the recorded video.
    To support live streaming we would have to make some tweaks to the videos service to allow a configurable extra channel to be used for live streaming by a user. In this case, it would also be best to make some UI changes to handle the case when there is no product metadata being sent alongside the stream (unless this functionality was also built into some sort of streaming interface).

  2. One for discussion on a call with yourself, Krithika and our team.

  3. We could not find any regression tests in the code base but we tested a variety of functionality including:

  • Product display and search.
  • Recommendations - user, item, search reranking.
  • Default recommendations.
  • Profile save + authentication.
  • Checkout.
  • Experimentation in recommendations.

We are currently testing:

  • Pinpoint events/messaging (but we do not expect any surprises there).
  1. To be clear, it is not necessarily a problem with the CloudFormation templates, but there was a problem whereby the ElasticSearch index failed to deploy due to a service issue in the regions we were using (we had a few issues with sticky resources, long creation times etc.). We do not have clear steps to reproduce, since after a few days ElasticSearch appeared to be functioning properly again.

@james-jory james-jory linked an issue Oct 27, 2020 that may be closed by this pull request
damiendaemon and others added 16 commits November 20, 2020 15:50
Updates:

Merged the Notebook of Discounts (1.3) to the Notebook of Personalise (1.1)
Renamed delete notebook to 1.3
This work was done by Dae.mn Team.
Co-authored-by: Akram Dweikat akram@akramdweikat.com
Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
Co-authored-by: Damien Jade Duff damien.duff@daemonsolutions.com
Co-authored-by: Joe Major joe.major@dae.mn
Update: Bring master new commits to Daemn branch 

This work was done by Dae.mn Team.
Co-authored-by: Akram Dweikat akram@akramdweikat.com
Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
Co-authored-by: Damien Jade Duff damien.duff@daemonsolutions.com
Co-authored-by: Joe Major joe.major@dae.mn
Update mlsld to be up to date with master
Fix product stock behavior and home page recommendations
…Live design

This work was done by Dae.mn Team.
Co-authored-by: Akram Dweikat akram@akramdweikat.com
Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
Co-authored-by: Damien Jade Duff damien.duff@daemonsolutions.com
Co-authored-by: Joe Major joe.major@dae.mn
This work was done by Dae.mn Team.
Co-authored-by: Akram Dweikat akram@akramdweikat.com
Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
Co-authored-by: Damien Jade Duff damien.duff@daemonsolutions.com
Co-authored-by: Joe Major joe.major@dae.mn
This work was done by Dae.mn Team.
Co-authored-by: Akram Dweikat akram@akramdweikat.com
Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
Co-authored-by: Damien Jade Duff damien.duff@daemonsolutions.com
Co-authored-by: Joe Major joe.major@dae.mn
This work was done by Dae.mn Team.
Co-authored-by: Akram Dweikat akram@akramdweikat.com
Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
Co-authored-by: Damien Jade Duff damien.duff@dae.mn
Co-authored-by: Joe Major joe.major@dae.mn
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.

Looking pretty good. Just some specific comments/changes.

Also, be sure to resize the screenshot added to the Personalize workshop so it matches the resolution of the other screenshots. This will keep the repo size down (and below the 20MB max for seeding the CodeCommit repo).

aws/cloudformation-templates/base/notebook.yaml Outdated Show resolved Hide resolved
aws/cloudformation-templates/base/ssm.yaml Outdated Show resolved Hide resolved
aws/cloudformation-templates/deployment-support.yaml Outdated Show resolved Hide resolved
src/recommendations/src/recommendations-service/app.py Outdated Show resolved Hide resolved
src/recommendations/src/recommendations-service/app.py Outdated Show resolved Hide resolved
FFMPEG_SUBS_COMMAND = "ffmpeg -i \"{video_filepath}\" \"{subtitle_path}\""

# -- Default stream details
# These details will be returned by the API if the 'use default IVS streams' option is chosen for deployment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to externalize the default stream details in a JSON file or something? Otherwise we're going to have to make changes to source when the product IDs change or when new/different default streams are added. Product IDs will be changing to UUIDs soon. I get that the defaults shouldn't change often but having the hardcoded makes it harder to maintain.

Changes to PR as requested including:
- Default streams JSON pulled from S3
- Capitalisation of IVS
- Small code and doc changes
- Bump Golang version - Due to dependency update.

This work was done by Dae.mn Team.
Co-authored-by: Akram Dweikat akram@akramdweikat.com
Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
Co-authored-by: Damien Jade Duff damien.duff@dae.mn
Co-authored-by: Joe Major joe.major@dae.mn
@damiendaemon
Copy link
Contributor

Hi all. We have made the latest requests changes and updated this PR. We also needed to bump the version of Golang in the products service (because of: jinzhu/copier#68 ) to allow this to deploy. Take care.

@james-jory
Copy link
Contributor

Hi all. We have made the latest requests changes and updated this PR. We also needed to bump the version of Golang in the products service (because of: jinzhu/copier#68 ) to allow this to deploy. Take care.

Changes look good. Just need to address conflict with the copier fix in the products service. We have an issue to move all golang services to use modules so thanks for doing this for the products service. A PR was merged yesterday to remove copier as a dependency of this service as a quick fix. Either back out your changes to the products service or merge your changes with upstream (to remove copier and add your module improvement).

Thanks!

damiendaemon and others added 4 commits December 2, 2020 08:09
Merge removal of copier dependency from master.

This work was done by Dae.mn Team.
Co-authored-by: Akram Dweikat akram@akramdweikat.com
Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
Co-authored-by: Damien Jade Duff damien.duff@daemonsolutions.com
Co-authored-by: Joe Major joe.major@dae.mn
Resize screenshot of discounts UI for notebook
 
This work was done by Dae.mn Team.
 Co-authored-by: Akram Dweikat akram@akramdweikat.com
 Co-authored-by: Akram Dweikat akram.dweikat@dae.mn
 Co-authored-by: Damien Jade Duff damien.duff@dae.mn
 Co-authored-by: Joe Major joe.major@dae.mn
…s/retail-store-demo into pr/Daemn-IVSdelivery
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.

LGTM. Thanks!

@james-jory james-jory merged commit 6dab660 into aws-samples:master Dec 2, 2020
@damiendaemon damiendaemon deleted the PR/Daemn-IVSdelivery branch November 10, 2021 12:29
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.

Integrate Amazon IVS
8 participants