Skip to content

LG-12342: Add files for Acuant SDK v11.9.3 + update documentation#10283

Merged
night-jellyfish merged 5 commits intomainfrom
brittany/lg-12342-prep-sdk-11.9.3
Mar 28, 2024
Merged

LG-12342: Add files for Acuant SDK v11.9.3 + update documentation#10283
night-jellyfish merged 5 commits intomainfrom
brittany/lg-12342-prep-sdk-11.9.3

Conversation

@night-jellyfish
Copy link
Contributor

@night-jellyfish night-jellyfish commented Mar 22, 2024

🎫 Ticket

LG-12342

🛠 Summary of changes

6092eabdf665c45a6d6e5f07c49acb07cbad62b7 adds the new v11.9.3 Acuant SDK files
aadf80ed3e9a4085b59cf939d9131791da68eb2b updates the documentation on how to test new SDK versions

📜 Testing Plan

  • Review the documentation added in aadf80ed3e9a4085b59cf939d9131791da68eb2b to see if it's clear
  • Try following the steps described in step 7 added in the commit linked above for at least one device / browser combination and make sure the SDK loads / works as expected

👀 Screenshots

Tested:

device chrome firefox safari
ios
android N/A N/A

@aduth
Copy link
Contributor

aduth commented Mar 22, 2024

Not sure if it's caused by the new files downloaded as of #10273, but this is 44MB of files. I think we really need to find a different way to download these files other than storing them in source control, since it's unsustainable to increase the clone size of the repository by 40+MB for every Acuant version bump.

@night-jellyfish
Copy link
Contributor Author

@aduth do you mind pointing me to where you see the size? I tried clicking around a bit and I don't see how to do that.

I think you're right, maybe there is a more ideal way, like a submodule or something? But I will say our practice has been:

  1. add new files
  2. a/b test in staging
  3. if all good, move to 100% and test in prod
  4. if all good, set default to new version, delete the files for 2 versions back (so that we always have 1 alternate to turn back to)

So while this does add a lot of files, we will also be deleting a lot (everything from 11.9.1) in a future PR, assuming all goes well.

@aduth
Copy link
Contributor

aduth commented Mar 22, 2024

@aduth do you mind pointing me to where you see the size? I tried clicking around a bit and I don't see how to do that.

I did a du -h public/acuant/11.9.3 in my Terminal

@aduth
Copy link
Contributor

aduth commented Mar 22, 2024

So while this does add a lot of files, we will also be deleting a lot (everything from 11.9.1) in a future PR, assuming all goes well.

If my understanding of how git operates is correct, because these files are part of the git history, their size contribution is permanent† for every git clone moving forward, even if the files are removed in the future.

† Except if main is rewritten/rebased or someone clones with specific clone depth, both of which are unlikely

@aduth
Copy link
Contributor

aduth commented Mar 22, 2024

I think you're right, maybe there is a more ideal way, like a submodule or something?

In the past I'd toyed with the idea of trying to define it as a NPM dependency (related Slack discussion).

We also have a number of other similar "depenencies" that we pull in as part of deploy and keep out of source control, such as pwned passwords, GeoMind IP data, disposable email domains. Maybe we can do something similar for Acuant.

@zachmargolis
Copy link
Contributor

I think you're right, maybe there is a more ideal way, like a submodule or something?

In the past I'd toyed with the idea of trying to define it as a NPM dependency (related Slack discussion).

We also have a number of other similar "depenencies" that we pull in as part of deploy and keep out of source control, such as pwned passwords, GeoMind IP data, disposable email domains. Maybe we can do something similar for Acuant.

+1 I think that if npm module is not sufficient, copying the files to our versioned S3 bucket seems like a good alternative for reliable deploys

@amirbey amirbey requested review from amirbey and removed request for amirbey March 22, 2024 19:38
@night-jellyfish night-jellyfish requested review from a team and eileen-nava and removed request for a team March 22, 2024 23:17
@night-jellyfish
Copy link
Contributor Author

If my understanding of how git operates is correct, because these files are part of the git history, their size contribution is permanent† for every git clone moving forward, even if the files are removed in the future.

Woah! I didn't expect that to be the case. Hmm.

These are good points. I was hoping to make a ticket for that, but while I was finally able to log into Jira today, I am now getting a 500 error.

Given upcoming release dates I wonder if we want to:

  • continue with this as we have been
  • write a ticket to revisit this
  • once release dates are past, come back to this ticket

or

  • since this size could have a permanent impact on the project's cloning time, if we really need to prioritize this now.

I could be missing something, but I do think there is a concern with this that I don't think (?) is relevant to the other cases you mentioned, such as pwned passwords. That concern is local dev. We really need the acuant code locally. If we were to put it in an S3 bucket, I don't think we could run the app on our contractor laptops. Perhaps NPM would solve that though.

I will bring this up to our standup on Monday.

@aduth
Copy link
Contributor

aduth commented Mar 25, 2024

I think the issue has always existed but hadn't been pressing enough to prioritize, especially when new releases weren't too frequent. I think with this new expected size it's becoming more urgent, since 44MB+ per new version is pretty excessive.

I found a command to check total git size contribution of a folder and it looks like Acuant has accumulated a total of 66MB to date. That's fairly large on its own, but I imagine that will quickly snowball with these new releases.

$ git rev-list --objects --all -- public/acuant | git cat-file --batch-check="%(objectsize) %(rest)" | cut -d" " -f1 | paste -s -d + - | bc | numfmt --to=si

66M

@night-jellyfish night-jellyfish force-pushed the brittany/lg-12342-prep-sdk-11.9.3 branch from 409e3fe to 43761d7 Compare March 25, 2024 17:17
@night-jellyfish
Copy link
Contributor Author

night-jellyfish commented Mar 25, 2024

@aduth thanks for bringing this to our attention. After talking at standup and in Slack, we found we could delete the two largest (by far) files (imageMagick.mjs and imageMagick.umd.js).

I wasn't sure if keeping the commit adding them would also add to git size (?) so I rebased and squashed their removal.

We do think it's beyond our current priorities to prioritize streamlining the use of Acuant files before upcoming deadlines. But with this change we go from 44MB --> 5.3MB to make this update now.

Then we can prioritize investigating another way to include Acuant files after upcoming deadlines (and hopefully before the next Acuant release). In the meantime I'll also put up a PR updating the download script so that we don't download these two files.

@aduth
Copy link
Contributor

aduth commented Mar 25, 2024

That's a good discovery to save the bulk of the size addition 👍 I think that's reasonable enough to put aside the immediate blocking concern.

Could you share the ticket for the follow-up solution, so that I can follow its progress?

night-jellyfish pushed a commit that referenced this pull request Mar 25, 2024
[See details on the reason for this change in this PR](#10283)

[skip changelog]
@night-jellyfish
Copy link
Contributor Author

@aduth Definitely! LG-9610.

And #10303 updates the download script.

night-jellyfish pushed a commit that referenced this pull request Mar 25, 2024
[See details on the reason for this change in this PR](#10283)

[skip changelog]
@amirbey amirbey self-requested a review March 27, 2024 17:16
@night-jellyfish night-jellyfish merged commit 578fcfb into main Mar 28, 2024
@night-jellyfish night-jellyfish deleted the brittany/lg-12342-prep-sdk-11.9.3 branch March 28, 2024 00:09
@aduth aduth mentioned this pull request Apr 2, 2024
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