Introduces autoloading strategy for service gems #3105

Schwad · 2024-09-16T15:48:10Z

Context: #3098

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

To make sure we include your contribution in the release notes, please make sure to add description entry for your changes in the "unreleased changes" section of the CHANGELOG.md file (at corresponding gem). For the description entry, please make sure it lives in one line and starts with Feature or Issue in the correct format.
For generated code changes, please checkout below instructions first:
https://github.com/aws/aws-sdk-ruby/blob/version-3/CONTRIBUTING.md

Thank you for your contribution!

This reverts commit 2b8a515.

alextwoods · 2024-09-16T21:10:36Z

@Schwad - I've created a pr in aws-sdk-rails: aws/aws-sdk-rails#140

Does this look like the correct/equivalent strategy for eager loading after this PR?

Schwad · 2024-09-17T10:18:20Z

@alextwoods Thanks for hopping onto this PR! We got it working in the end. :)

This looks correct to me:

Add Aws into eager_load_namespaces for Rails
Define eager_load!
Doubly-nest the const_get so we're essentially eager loading those top-level constants for each of the service-level gems

I haven't manually checked yet, but if you're ever unsure if it's working you can set config.eager_load = true in development.rb, while pointing to this copy of the gem, and throw a puts statement right inside Aws::S3::Bucket to see it is indeed eager loaded on boot. (And not when you set eager_load = false).

Sorry, you probably knew that all, just talking this through out loud 😅

Schwad · 2024-09-17T10:18:46Z

And this PR is looking even more manageable without all those Railties added.

alextwoods · 2024-09-17T16:08:31Z

I think this is looking pretty good! I've completed testing with all of our internal build systems and everything looks good. I'll discuss with the rest of the team, but I'd love to get this released this week if possible. There are some additional optimizations I'd like to do (particularly around aws-partitions), but those should probably be done as a separate PR.

Thanks for the testing suggestion for the aws-sdk-rails change - its good confirmation that what I was doing to test makes sense. Could you review/approve that PR? (I'll also have another other of the SDK developers review, but since you originally wrote the railties here).

alextwoods · 2024-09-17T16:15:31Z

gems/aws-sdk-core/spec/aws/cbor/cbor_engine_spec.rb

@@ -1,4 +1,5 @@
 require_relative '../../spec_helper'
+require 'aws-sdk-core/cbor/cbor_engine'


Note: These are required because of the way engines are loaded (Ie, we don't just reference the CborEngine, we do a require on the file when trying to determine the default engine. This means it does not play well with autoloads).

Can spec_helper load all the stuff?

mullermp

Nice so far!

mullermp · 2024-09-17T16:16:59Z

build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/views/errors_module.rb

      def module_name
        @service.module_name
      end

+      # @return [Boolean]
+      def customization_file_exists?
+        File.exist?(File.join(__dir__, "../../../../../gems/aws-sdk-#{gem_name}/lib/aws-sdk-#{gem_name}/customizations/errors.rb"))


Implementation of this can probably call customization_file_path

cleaned that up in all views + created a helper for the gem lib path.

mullermp · 2024-09-17T16:17:38Z

build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/views/root_resource_class.rb

@@ -18,6 +18,7 @@ def initialize(options)
          paginators: options.fetch(:paginators)
        )
        @custom = options.fetch(:custom)
+        @name = @module_name.split('::').last.downcase


Is this gem name? Why do the other classes have a method and this not?

Yeah good call - will refactor it to follow pattern of other modules.

build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/views/service_module.rb

mullermp · 2024-09-17T16:19:30Z

build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/views/service_module.rb

+          class_name = File.basename(path).split('.').first.split('_').map(&:capitalize).join
+
+          # Handle the Db -> DB case for AWS database-related constants
+          class_name = class_name.gsub(/Db(?=[A-Z]|$)/, 'DB')


There are many inflections that need to be handled.. I don't like this as a one-off. Consider somehow using the inflections from the underscore method and verify nothing bad happens?

I've actually completely changed this method so that it should not need to do this class name inference and hopefully made it more readable.

build_tools/aws-sdk-code-generator/templates/types_module.mustache

mullermp · 2024-09-17T16:22:13Z

gems/aws-sdk-cloudfront/spec/url_signer_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true

 require_relative 'spec_helper'
+require 'aws-sdk-cloudfront/customizations'


Do all spec files need to require customizations now? Could instead a generated spec helper (or somehow the shared one) do this?

I think this was legacy of some other issues that have been fixed. Removed from here.

mullermp · 2024-09-17T16:22:33Z

gems/aws-sdk-core/CHANGELOG.md

@@ -1,6 +1,8 @@
 Unreleased Changes
 ------------------

+* Feature - Use autoloading at the service level to load service clients and resources.


Do all service gems need to be bumped to use this new version of core?

No -I don't think they need to. New service gems can still use the old core without issue (and vice versa).

mullermp · 2024-09-17T16:23:46Z

gems/aws-sdk-core/lib/aws-sdk-core.rb

+  autoload :Util, 'aws-sdk-core/util'
+
+  # resource classes
+  module Resources


Should each of these modules be pushed into a different file? Maybe resources.rb, plugins.rb, log.rb etc?

yeah, thats a good idea. Moved all of these to their own files.

mullermp · 2024-09-17T16:24:15Z

gems/aws-sdk-core/spec/aws/cbor/decoder_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true

 require_relative '../../spec_helper'
+require 'aws-sdk-core/cbor/decoder'


Why are all these required for specs to work?

See other answer about engines. (we do an explicit require in the cbor engine when setting a default. this does not work with autoload).

gems/aws-sdk-ec2/lib/aws-sdk-ec2/customizations.rb

mullermp

Mostly nits and maybe you forgot some stuff?

build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/views/service_module.rb

build_tools/aws-sdk-code-generator/templates/types_module.mustache

mullermp · 2024-09-17T22:20:14Z

gems/aws-sdk-core/spec/aws/cbor_spec.rb

@@ -1,6 +1,8 @@
 # frozen_string_literal: true

 require_relative '../spec_helper'
+require 'aws-sdk-core/cbor'


Extra newline

gems/aws-sdk-iam/lib/aws-sdk-iam/customizations.rb

mullermp · 2024-09-17T22:22:24Z

gems/aws-sdk-translate/spec/translate_document_encoding_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true

 require_relative 'spec_helper'
+require_relative '../lib/aws-sdk-translate/plugins/translate_document_encoding'


Is this still necessary to require? Other plugins don't need this so maybe an issue.

Moved this to an autoload in the customizations. (Note, this only impacts cases where the client isn't used before trying to access this custom plugin, as in this test).

Schwad · 2024-09-18T08:47:53Z

I live in the UK so sorry I can't always reply quickly, but the direction of the conversation here looks right to me, no protests or disagreements. I'll throw down a review/approval on the aws-sdk-rails PR 🙇

alextwoods · 2024-09-23T20:17:57Z

This was released today! New versions of each service gem were published eg: aws-sdk-s3@1.165.0 and aws-sdk-core@3.208.0.

Thanks again for a great contribution! I'll work on getting the eager load feature out in aws-sdk-rails over the next few days.

Schwad · 2024-09-25T02:31:42Z

That's wonderful news!! Thank you for the wonderful collaboration. I truly found this a joy to work on and am so happy with the reception to the contribution. Cheers again!

We freeze everything to catch malicious self modifying code from user input. Since the Aws SDK started to autoload modules when used, so we need to load them before freezing. aws/aws-sdk-ruby#3105

Schwad and others added 12 commits September 6, 2024 12:28

Changes code-generation to use autoloading with service modules

f3dbfe1

Manual autoloading introduced for customizations and aws-sdk-core

82d6b54

Updates codegenerator to require railtie if defined

f16cc3d

Adds railties to services

44fd76d

Updates for specs

796356c

Generated code using bundle exec rake build

2b8a515

Revert "Generated code using bundle exec rake build"

3702029

This reverts commit 2b8a515.

Remove railties

11139ba

Merge branch 'version-3' into schwad_autoload

8e7d8db

Fix failing tests

b46bc2c

Fix build system failures

4b2e6a4

Move changelog to core

3a756c6

Schwad mentioned this pull request Sep 16, 2024

Introduces autoloading strategy for service gems #3098

Closed

alextwoods self-assigned this Sep 16, 2024

aws-sdk-ruby-automation added 5 commits September 16, 2024 09:12

Skip uploading benchmark metrics when credentials aren't available

aea96fe

Allow continue-on-error for benchmark credentials

2ea74a4

Remove uneeded requires in cbor specs

df22114

Use autoload for more namespaces in core

242e7e1

Move bundled gem requires until after autoload setup

7b95851

alextwoods mentioned this pull request Sep 16, 2024

SDK Eager loading aws/aws-sdk-rails#140

Merged

alextwoods approved these changes Sep 17, 2024

View reviewed changes

alextwoods requested a review from mullermp September 17, 2024 16:08

alextwoods reviewed Sep 17, 2024

View reviewed changes

mullermp reviewed Sep 17, 2024

View reviewed changes

aws-sdk-ruby-automation and others added 2 commits September 17, 2024 11:38

PR Cleanups

7a7b372

Merge branch 'version-3' into schwad_autoload

2cb5937

aws-sdk-ruby-automation and others added 4 commits September 17, 2024 11:51

Add stubbing autoload

f1fe00d

Add autoload for resources, cleanup require order

abbc1d8

Add autoloading for log module

8057c80

Merge branch 'version-3' into schwad_autoload

5690ebb

mullermp approved these changes Sep 17, 2024

View reviewed changes

PR Cleanups 2

790ddd1

Merge branch 'version-3' into schwad_autoload

10ecce8

alextwoods merged commit 94d4416 into aws:version-3 Sep 23, 2024
31 checks passed

alextwoods deleted the schwad_autoload branch September 23, 2024 16:36

alextwoods mentioned this pull request Sep 24, 2024

aws-sdk-s3 version 1.165.0 invalid configuration option #3113

Closed

1 task

enescakir mentioned this pull request Oct 2, 2024

Load Aws::S3 module before freezing them ubicloud/ubicloud#2010

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces autoloading strategy for service gems #3105

Introduces autoloading strategy for service gems #3105

Schwad commented Sep 16, 2024

alextwoods commented Sep 16, 2024

Schwad commented Sep 17, 2024

Schwad commented Sep 17, 2024

alextwoods commented Sep 17, 2024

alextwoods Sep 17, 2024

mullermp Sep 17, 2024

mullermp left a comment

mullermp Sep 17, 2024

alextwoods Sep 17, 2024

mullermp Sep 17, 2024

alextwoods Sep 17, 2024

mullermp Sep 17, 2024

alextwoods Sep 17, 2024

mullermp Sep 17, 2024

alextwoods Sep 17, 2024

mullermp Sep 17, 2024

alextwoods Sep 17, 2024

mullermp Sep 17, 2024

alextwoods Sep 17, 2024

mullermp Sep 17, 2024

alextwoods Sep 17, 2024

mullermp left a comment

mullermp Sep 17, 2024

mullermp Sep 17, 2024

alextwoods Sep 17, 2024 •

edited

Loading

Schwad commented Sep 18, 2024

alextwoods commented Sep 23, 2024

Schwad commented Sep 25, 2024

		@@ -1,4 +1,5 @@
		require_relative '../../spec_helper'
		require 'aws-sdk-core/cbor/cbor_engine'

Introduces autoloading strategy for service gems #3105

Introduces autoloading strategy for service gems #3105

Conversation

Schwad commented Sep 16, 2024

alextwoods commented Sep 16, 2024

Schwad commented Sep 17, 2024

Schwad commented Sep 17, 2024

alextwoods commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullermp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullermp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextwoods Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Schwad commented Sep 18, 2024

alextwoods commented Sep 23, 2024

Schwad commented Sep 25, 2024

alextwoods Sep 17, 2024 •

edited

Loading