Feature Request: Lazy loading subcommands? Shave of ms! (maybe)

[emilebosch]

Hey all, I'm trying to shave off as much time as I can in Thor. Therefore I was thinking is there any interest in lazy loading subcommands?

As far as I know the sub commands are only needed to be loaded when:

• something starts with the subcommand
• Someone wants to introspect the commands

The first idea was to make the subcommand a lambda:

 desc "beers", "Creates and manages beers"
 subcommand "beer", -> {Beer}

For the people that want to introspect the commands they can use eager load (probably breaking) or we can eager load by default unless lazy loading is set too true somewhere.

[rafaelfranca]

I'm not against the idea, but to really consider it I need to see the code and how much time that would save. Do you think you could build a prototype and measure the impact?

[emilebosch]

👋 Hi Rafael! Nice to hear from you. It really depends on what the sub commands load of course. My subcommands are provided by other gems so I don't really have influence of their loading behaviour.

I do get your point tho. I can also just tell my other gem implementers to have good gem manners and use autoload.

[hlascelles]

We have exactly the same feature requirement, and we have already put together a working version.

We are using String -> Constant, but I like the idea of a block/proc too.

There are about 15 subcommands, and it takes ~2 sec to load them all. With this optimisation it goes to 0.4 sec. We run them a lot, so it adds up. We would like this feature if it were made "formal".

We could also switch to Autoload to reduce the subcommand task load times, that is true.

Until then, this works great for us. We have a file our_bin which has a ruby shebang at the top we use to invoke the commands.

  class << self
    def possible_subcommands(using_bin, requested_subcommand)
      possible = {
        "boo" => ["Boo!", "Tasks::Boo"],
        "baz" => ["Doing Baz", "Tasks::Baz"],
      }
      # Here we only "require" the necessary subcommand if we have to. If an unknown
      # subcommand (including "help") or no subcommand is requested, we load everything.
      # NB, if we are running specs on this code, then using_bin is false.
      load_commands =
        if using_bin && possible.keys.include?(requested_subcommand)
          possible.slice(requested_subcommand)
        else
          # If we aren't certain of the subcommand, or if not using the bin file, load everything
          possible
        end
      load_commands.map { |name, options|
        require_relative "tasks/#{name}"
        [name, [options[0], Object.const_get(options[1])]]
      }.to_h
    end
  end

  possible_subcommands($PROGRAM_NAME.end_with?("our_bin"), ARGV.first)
    .each do |subcommand_name, (description, clazz)|
      desc subcommand_name, description
      subcommand subcommand_name, clazz
    end

[emilebosch]

Hi Harry good stuff! Happy to see you have the same issue! I lean towards proc since it prevents magical loading or looking up constants from strings which will also hurt you in the buttocks when refactoring. I.e a string error makes your app go boom.

Maybe we should go fork it and provide a PR? 😬

[hlascelles]

Yes, I agree a proc sounds good.

Absolutely, yes let's go for a PR. Would you like to do the honours or shall I?

And just checking, @rafaelfranca, given my single point of data (2 sec -> 0.4 sec real world example), would you be willing to receive it (if it passes code muster of course)?

[josacar]

Personally I like the idea of knife from chef: https://github.com/chef/chef/blob/main/knife/lib/chef/knife.rb#L240-L242 so you can add them dependencies in run-time https://github.com/chef/chef/blob/61a11902ab814aad3625eb4da7e3345d63ee7c09/knife/lib/chef/knife/exec.rb#L26-L28

This to avoid big clis using for example different AWS SDK parts can shave a lot of time. What do you think?

[dorner]

I think it might be a bit overkill - moving the subcommands into a proc sounds like it would solve the same problem in a way that would be a bit simpler for command developers to grok. But I'm not dead against it.

[hlascelles]

NB, we also now take into account using unique command prefixes (which Thor supports).

Our new code is:

  class << self
    def possible_subcommands(using_bin, requested_subcommand)
      all_commands = {
        "boo" => ["Boo!", "Tasks::Boo"],
        "baz" => ["Doing Baz", "Tasks::Baz"],
      }
      # Here we only "require" the necessary subcommand if we have to. If an unknown
      # subcommand (including "help") or no subcommand is requested, we load everything.
      # NB, if we are running specs on this code, then using_bin is false.
      matched_commands = all_commands.select { |k, _| k.start_with?(requested_subcommand || "") }
      load_commands = using_bin && matched_commands.size == 1 ? matched_commands : all_commands
      load_commands.map { |name, options|
        require_relative "tasks/#{name}"
        [name, [options[0], Object.const_get(options[1])]]
      }.to_h
    end
  end

  possible_subcommands($PROGRAM_NAME.end_with?("our_bin"), ARGV.first)
    .each do |subcommand_name, (description, clazz)|
      desc subcommand_name, description
      subcommand subcommand_name, clazz
    end

[josacar]

Hi, I was hacking on this feature and I have a hacky PR that implements this here main...josacar:thor:main

The thing is deps gets done for all commands inside a Thor class but help, this can be done to do it as per-command basis too, but I want to get some feedback here as it really shaves a lot of time.

I dunno if autoload or Zeitwerk can also be a feasible solution to avoid this.