ronantakizawa/codereview-bench huggingface dataset:
Repository: mlc-ai/mlc-llm
File: python/mlc_llm/support/auto_target.py (Python)

Code before:
            "kind": "opencl",
            "host": {
                "kind": "llvm",
                "mtriple": "aarch64-linux-android",
            },
        },
        "build": _build_android,
    },
    "android:adreno": {
        "target": {
            "kind": "opencl",
            "device": "adreno",
            "host": {
                "kind": "llvm",
                "mtriple": "aarch64-linux-android",
            },
        },
        "build": _build_android,
    },
    "android:adreno-so": {
        "target": {
            "kind": "opencl",
            "device": "adreno",
            "host": {
                "kind": "llvm",
                "mtriple": "aarch64-linux-android",
            },
        },
        "build": _build_android_so,
    },
    "metal:x86-64": {
        "target": {
            "kind": "metal",
            "max_threads_per_block": 256,
            "max_shared_memory_per_block": 32768,
            "thread_warp_size": 1,
        },
        "build": _build_metal_x86_64,
    },
    "webgpu:generic": {
        "target": {
            "kind": "webgpu",
            "host": {
                "kind": "llvm",
                "mtriple": "wasm32-unknown-unknown-wasm",
            },
        },
        "build": _build_webgpu,
    },
    "opencl:generic": {
        "target": {

Review comment (suggestion):
@tqchen 
We have reverted the generic opencl max thread changes https://github.com/apache/tvm/pull/16972 
We have override the max-thread =512 for android:adreno target in mlc.

Code after:
            "kind": "opencl",
            "host": {
                "kind": "llvm",
                "mtriple": "aarch64-linux-android",
            },
        },
        "build": _build_android,
    },
    "android:adreno": {
        "target": {
            "kind": "opencl",
            "device": "adreno",
            "max_threads_per_block": 512,
            "host": {
                "kind": "llvm",
                "mtriple": "aarch64-linux-android",
            },
        },
        "build": _build_android,
    },
    "android:adreno-so": {
        "target": {
            "kind": "opencl",
            "device": "adreno",
            "max_threads_per_block": 512,
            "host": {
                "kind": "llvm",
                "mtriple": "aarch64-linux-android",
            },
        },
        "build": _build_android_so,
    },
    "metal:x86-64": {
        "target": {
            "kind": "metal",
            "max_threads_per_block": 256,
            "max_shared_memory_per_block": 32768,
            "thread_warp_size": 1,
        },
        "build": _build_metal_x86_64,
    },
    "webgpu:generic": {
        "target": {
            "kind": "webgpu",
            "host": {
                "kind": "llvm",
                "mtriple": "wasm32-unknown-unknown-wasm",
            },
        },
        "build": _build_webgpu,
    },
Repository: mrsked/mrsk
File: lib/mrsk/cli/healthcheck.rb (Ruby)

Code before:
class Mrsk::Cli::Healthcheck < Mrsk::Cli::Base
  MAX_ATTEMPTS = 7

  class HealthcheckError < RuntimeError
    def initialize(message)
      super "Health check against #{MRSK.config.healthcheck["path"]} #{message}"
    end
  end

  default_command :perform

  desc "perform", "Health check current app version"
  def perform
    on(MRSK.primary_host) do
      begin
        execute *MRSK.healthcheck.run

        attempt = 1
        begin
          case output = capture_with_info(*MRSK.healthcheck.curl, raise_on_non_zero_exit: false)
          when "200"
            true
          when "000"
            raise HealthcheckError, "failed to connect"
          when /\A\d+\z/
            raise HealthcheckError, "failed with status #{output}"
          else
            raise HealthcheckError, "failed to run: #{output}"
          end
        rescue HealthcheckError => error
          if attempt <= MAX_ATTEMPTS
            info "#{error.message}, retrying in #{attempt}s..."
            sleep attempt
            attempt += 1
            retry
          else
            raise
          end
        end

        info "Health check succeeded with 200 OK!"
      rescue HealthcheckError => e
        error capture_with_info(*MRSK.healthcheck.logs)
        raise
      ensure

Review comment (bug):
We were raising on non-200-OK curl responses previously, bypassing the status parsing conditions below.

Instead, don't raise, eliminating `SSHKit::Command::Failed` handling, giving us just retries on `HealthcheckError` to worry about.

Code after:
class Mrsk::Cli::Healthcheck < Mrsk::Cli::Base
  default_command :perform

  desc "perform", "Health check current app version"
  def perform
    on(MRSK.primary_host) do
      begin
        execute *MRSK.healthcheck.run
        Mrsk::Utils::HealthcheckPoller.wait_for_healthy { capture_with_info(*MRSK.healthcheck.status) }
      rescue Mrsk::Utils::HealthcheckPoller::HealthcheckError => e
        error capture_with_info(*MRSK.healthcheck.logs)
        raise
      ensure
        execute *MRSK.healthcheck.stop, raise_on_non_zero_exit: false
        execute *MRSK.healthcheck.remove, raise_on_non_zero_exit: false
      end
    end
  end
end
Repository: mrsked/mrsk
File: lib/mrsk/cli/server.rb (Ruby)

Code before:
class Mrsk::Cli::Server < Mrsk::Cli::Base
  desc "bootstrap", "Ensure readiness to run MRSK"
  def bootstrap
    unready = []

    on(MRSK.hosts + MRSK.accessory_hosts) do |host|
      unready << host unless execute(:docker, :info, raise_on_non_zero_exit: false)
    end

    if unready.any?
      raise "Docker is not running on #{unready.join(", ")}. Install it first: https://docs.docker.com/engine/install/"
    end
  end
end


Review comment (suggestion):
Indicate how to install Docker rather than automating it. That only worked with SSH root users—which most hosts strongly discourage—and on Debian/Ubuntu hosts. Clearer to indicate what needs to be set up. Allows MRSK to remain indifferent to deploy target platform.

Code after:
class Mrsk::Cli::Server < Mrsk::Cli::Base
  desc "bootstrap", "Set up Docker to run MRSK apps"
  def bootstrap
    missing = []

    on(MRSK.hosts | MRSK.accessory_hosts) do |host|
      unless execute(*MRSK.docker.installed?, raise_on_non_zero_exit: false)
        if execute(*MRSK.docker.superuser?, raise_on_non_zero_exit: false)
          info "Missing Docker on #{host}. Installing…"
          execute *MRSK.docker.install
        else
          missing << host
        end
      end
    end

    if missing.any?
      raise "Docker is not installed on #{missing.join(", ")} and can't be automatically installed without having root access and the `curl` command available. Install Docker manually: https://docs.docker.com/engine/install/"
    end
  end
end
Repository: mrsked/mrsk
File: lib/mrsk/commands/healthcheck.rb (Ruby)

Code before:
class Mrsk::Commands::Healthcheck < Mrsk::Commands::Base
  EXPOSED_PORT = 3999

  CURL_ARGS = [ "--silent", "--output", "/dev/null", "--write-out", "'%{http_code}'", "--max-time", "2" ]

  def run
    web = config.role(:web)

    docker :run,
      "--detach",
      "--name", container_name_with_version,
      "--publish", "#{EXPOSED_PORT}:#{config.healthcheck["port"]}",
      "--label", "service=#{container_name}",
      "-e", "MRSK_CONTAINER_NAME=\"#{container_name}\"",
      *web.env_args,
      *config.volume_args,
      *web.option_args,
      config.absolute_image,
      web.cmd
  end

  def curl
    [ *config.healthcheck.fetch("curl"), *CURL_ARGS, health_url ]
  end

  def logs
    pipe container_id, xargs(docker(:logs, "--tail", 50, "2>&1"))
  end

  def stop
    pipe container_id, xargs(docker(:stop))
  end

  def remove
    pipe container_id, xargs(docker(:container, :rm))
  end

  private
    def container_name
      [ "healthcheck", config.service, config.destination ].compact.join("-")
    end

    def container_name_with_version
      "#{container_name}-#{config.version}"
    end

    def container_id
      container_id_for(container_name: container_name_with_version)

Review comment (suggestion):
`curl` option now defaults to a `docker` invocation, limiting MRSK deps to Docker alone. No native curl install required.

Code after:
class Mrsk::Commands::Healthcheck < Mrsk::Commands::Base
  EXPOSED_PORT = 3999

  def run
    web = config.role(:web)

    docker :run,
      "--detach",
      "--name", container_name_with_version,
      "--publish", "#{EXPOSED_PORT}:#{config.healthcheck["port"]}",
      "--label", "service=#{container_name}",
      "-e", "MRSK_CONTAINER_NAME=\"#{container_name}\"",
      *web.env_args,
      *web.health_check_args,
      *config.volume_args,
      *web.option_args,
      config.absolute_image,
      web.cmd
  end

  def status
    pipe container_id, xargs(docker(:inspect, "--format", DOCKER_HEALTH_STATUS_FORMAT))
  end

  def logs
    pipe container_id, xargs(docker(:logs, "--tail", 50, "2>&1"))
  end

  def stop
    pipe container_id, xargs(docker(:stop))
  end

  def remove
    pipe container_id, xargs(docker(:container, :rm))
  end

  private
    def container_name
      [ "healthcheck", config.service, config.destination ].compact.join("-")
    end

    def container_name_with_version
      "#{container_name}-#{config.version}"
    end

    def container_id
      container_id_for(container_name: container_name_with_version)
    end
Repository: mrsked/mrsk
File: lib/mrsk/cli/base.rb (Ruby)

Code before:
            commander.verbosity = :error
          end

          commander.configure \
            config_file: Pathname.new(File.expand_path(options[:config_file])),
            destination: options[:destination],
            version: options[:version]

          commander.specific_hosts    = options[:hosts]&.split(",")
          commander.specific_roles    = options[:roles]&.split(",")
          commander.specific_primary! if options[:primary]
        end
      end

      def print_runtime
        started_at = Time.now
        yield
        return Time.now - started_at
      ensure
        runtime = Time.now - started_at
        puts "  Finished all in #{sprintf("%.1f seconds", runtime)}"
      end

      def audit_broadcast(line)
        if broadcast = MRSK.auditor.broadcast(line)
          system(MRSK.auditor.broadcast_environment(line), broadcast)
        end
      end

      def with_lock
        if MRSK.holding_lock?
          yield
        else
          acquire_lock

          begin
            yield
          rescue
            if MRSK.hold_lock_on_error?
              error "  \e[31mDeploy lock was not released\e[0m"
            else
              release_lock
            end

            raise
          end

          release_lock
        end
      end


Review comment (suggestion):
I don't think there's a way to set the environment when using `run_locally`, so I switched to `system` for this one instead.

Code after:
            commander.verbosity = :error
          end

          commander.configure \
            config_file: Pathname.new(File.expand_path(options[:config_file])),
            destination: options[:destination],
            version: options[:version]

          commander.specific_hosts    = options[:hosts]&.split(",")
          commander.specific_roles    = options[:roles]&.split(",")
          commander.specific_primary! if options[:primary]
        end
      end

      def print_runtime
        started_at = Time.now
        yield
        return Time.now - started_at
      ensure
        runtime = Time.now - started_at
        puts "  Finished all in #{sprintf("%.1f seconds", runtime)}"
      end

      def audit_broadcast(line)
        run_locally { execute *MRSK.auditor.broadcast(line), verbosity: :debug }
      end

      def with_lock
        if MRSK.holding_lock?
          yield
        else
          acquire_lock

          begin
            yield
          rescue
            if MRSK.hold_lock_on_error?
              error "  \e[31mDeploy lock was not released\e[0m"
            else
              release_lock
            end

            raise
          end

          release_lock
        end
      end

      def acquire_lock
        say "Acquiring the deploy lock"
Repository: mrsked/mrsk
File: lib/mrsk/utils/healthcheck_poller.rb (Ruby)

Code before:
class Mrsk::Utils::HealthcheckPoller
  TRAEFIK_HEALTHY_DELAY = 1

  class HealthcheckError < StandardError; end

  class << self
    def wait_for_healthy(pause_after_ready: false, &block)
      attempt = 1
      max_attempts = MRSK.config.healthcheck["max_attempts"]

      begin
        case status = block.call
        when "healthy"
          sleep TRAEFIK_HEALTHY_DELAY if pause_after_ready
        when "running" # No health check configured
          sleep MRSK.config.readiness_delay if pause_after_ready
        else
          raise HealthcheckError, "container not ready (#{status})"
        end
      rescue HealthcheckError => e
        if attempt <= max_attempts
          info "#{e.message}, retrying in #{attempt}s (attempt #{attempt}/#{max_attempts})..."
          sleep attempt
          attempt += 1
          retry
        else
          raise

Review comment (bug):
@kevinmcconnell - the integration tests were consistently failing in Github actions until I upped this to 2 seconds. Traefik was returning a 404 when we checked if the app was up.

Worked fine with 1 second on my M1, so probably a resource constraint issue - I think its worth sticking with a slightly longer default delay, but maybe this should be configurable.

Code after:
class Mrsk::Utils::HealthcheckPoller
  TRAEFIK_HEALTHY_DELAY = 2

  class HealthcheckError < StandardError; end

  class << self
    def wait_for_healthy(pause_after_ready: false, &block)
      attempt = 1
      max_attempts = MRSK.config.healthcheck["max_attempts"]

      begin
        case status = block.call
        when "healthy"
          sleep TRAEFIK_HEALTHY_DELAY if pause_after_ready
        when "running" # No health check configured
          sleep MRSK.config.readiness_delay if pause_after_ready
        else
          raise HealthcheckError, "container not ready (#{status})"
        end
      rescue HealthcheckError => e
        if attempt <= max_attempts
          info "#{e.message}, retrying in #{attempt}s (attempt #{attempt}/#{max_attempts})..."
          sleep attempt
          attempt += 1
          retry
        else
          raise
Repository: mrsked/mrsk
File: lib/mrsk/commands/builder/native.rb (Ruby)

Code before:
    if config.builder.cache?
      docker :buildx, :create, "--use", "--driver=docker-container"
    end
  end

  def remove
    # No-op on native without cache

    if config.builder.cache?
      docker :buildx, :rm, builder_name
    end
  end

  def push
    if config.builder.cache?
      docker :buildx, :build,
        "--push",
        *build_options,
        build_context
    else
      combine \
        docker(:build, *build_options, build_context),
        docker(:push, config.absolute_image),
        docker(:push, config.latest_image)
    end
  end

  def info
    # No-op on native
  end
end


Review comment (performance):
Instead of doing conditionals on all of these, I think maybe we should have `Mrsk::Commands::Builder::Native::Cached`?

Code after:
class Mrsk::Commands::Builder::Native < Mrsk::Commands::Builder::Base
  def create
    # No-op on native without cache
  end

  def remove
    # No-op on native without cache
  end

  def push
    combine \
      docker(:build, *build_options, build_context),
      docker(:push, config.absolute_image),
      docker(:push, config.latest_image)
  end

  def info
    # No-op on native
  end
end
Repository: mrsked/mrsk
File: lib/mrsk/cli/build.rb (Ruby)

Code before:
class Mrsk::Cli::Build < Mrsk::Cli::Base
  class BuildError < StandardError; end

  desc "deliver", "Build app and push app image to registry then pull image on servers"
  def deliver
    mutating do
      push
      pull
    end
  end

  desc "push", "Build and push app image to registry"
  def push
    mutating do
      cli = self

      verify_local_dependencies
      run_hook "pre-build"

      if (uncommitted_changes = Mrsk::Utils.uncommitted_changes).present?
        say "The following paths have uncommitted changes (check your .gitignore file):\n #{uncommitted_changes}", :yellow
      end

      run_locally do
        begin
          MRSK.with_verbosity(:debug) do
            execute *MRSK.builder.push
          end
        rescue SSHKit::Command::Failed => e
          if e.message =~ /(no builder)|(no such file or directory)/
            error "Missing compatible builder, so creating a new one first"

            if cli.create
              MRSK.with_verbosity(:debug) { execute *MRSK.builder.push }
            end
          else
            raise
          end
        end
      end
    end
  end

  desc "pull", "Pull app image from registry onto servers"
  def pull
    mutating do

Review comment (suggestion):
You might also want to add the files to your `.dockerignore` to avoid them ending up in the image. I'm not sure we can get this down to a succinct message, so I think it's best to remove the `(check your .gitignore file)` from the message.

Code after:
class Mrsk::Cli::Build < Mrsk::Cli::Base
  class BuildError < StandardError; end

  desc "deliver", "Build app and push app image to registry then pull image on servers"
  def deliver
    mutating do
      push
      pull
    end
  end

  desc "push", "Build and push app image to registry"
  def push
    mutating do
      cli = self

      verify_local_dependencies
      run_hook "pre-build"

      if (uncommitted_changes = Mrsk::Utils.uncommitted_changes).present?
        say "The following paths have uncommitted changes:\n #{uncommitted_changes}", :yellow
      end

      run_locally do
        begin
          MRSK.with_verbosity(:debug) do
            execute *MRSK.builder.push
          end
        rescue SSHKit::Command::Failed => e
          if e.message =~ /(no builder)|(no such file or directory)/
            error "Missing compatible builder, so creating a new one first"

            if cli.create
              MRSK.with_verbosity(:debug) { execute *MRSK.builder.push }
            end
          else
            raise
          end
        end
      end
    end
  end

  desc "pull", "Pull app image from registry onto servers"
  def pull
    mutating do
Repository: mrsked/mrsk
File: lib/mrsk/configuration.rb (Ruby)

Code before:
  end

  def ssh_proxy
    if raw_config.ssh.present? && raw_config.ssh["proxy"]
      Net::SSH::Proxy::Jump.new \
        raw_config.ssh["proxy"].include?("@") ? raw_config.ssh["proxy"] : "root@#{raw_config.ssh["proxy"]}"
    elsif raw_config.ssh.present? && raw_config.ssh["proxy_command"]
      Net::SSH::Proxy::Command.new(raw_config.ssh["proxy_command"])
    end
  end

  def ssh_options
    { user: ssh_user, proxy: ssh_proxy, auth_methods: [ "publickey" ], keepalive: true, keepalive_interval: 30 }.compact
  end


  def sshkit_max_concurrent_starts
    raw_config.sshkit["max_concurrent_starts"] if raw_config.sshkit.present?
  end

  def sshkit_pool_idle_timeout
    if raw_config.sshkit.present?
      raw_config.sshkit["pool_idle_timeout"] || 900
    else
      900
    end
  end


  def healthcheck
    { "path" => "/up", "port" => 3000, "max_attempts" => 7 }.merge(raw_config.healthcheck || {})
  end

  def readiness_delay
    raw_config.readiness_delay || 7
  end

  def minimum_version
    raw_config.minimum_version
  end

  def valid?
    ensure_required_keys_present && ensure_valid_mrsk_version
  end


  def to_h
    {
      roles: role_names,
      hosts: all_hosts,
      primary_host: primary_web_host,

Review comment (refactor):
Since there are already a bunch of different SSH configuration methods, I think it would be better to extract them to a separate class similar to `Mrsk::Configuration::Accessory`, `Mrsk::Configuration::Builder` and others.

Code after:
  end


  def healthcheck
    { "path" => "/up", "port" => 3000, "max_attempts" => 7 }.merge(raw_config.healthcheck || {})
  end

  def readiness_delay
    raw_config.readiness_delay || 7
  end

  def minimum_version
    raw_config.minimum_version
  end

  def valid?
    ensure_required_keys_present && ensure_valid_mrsk_version
  end


  def to_h
    {
      roles: role_names,
      hosts: all_hosts,
      primary_host: primary_web_host,
      version: version,
      repository: repository,
      absolute_image: absolute_image,
      service_with_version: service_with_version,
      env_args: env_args,
      volume_args: volume_args,
      ssh_options: ssh.options,
      sshkit: sshkit.to_h,
      builder: builder.to_h,
      accessories: raw_config.accessories,
      logging: logging_args,
      healthcheck: healthcheck
    }.compact
  end

  def traefik
    raw_config.traefik || {}
  end

  def hooks_path
    raw_config.hooks_path || ".mrsk/hooks"
  end

  def builder
    Mrsk::Configuration::Builder.new(config: self)
  end
Repository: mrsked/mrsk
File: lib/kamal/commands/app.rb (Ruby)

Code before:
      [config.service, role, config.destination].compact.join("-")
    end

    def filters(statuses: nil)
      [ "label=service=#{config.service}" ].tap do |filters|
        filters << "label=destination=#{config.destination}" if config.destination
        filters << "label=role=#{role}" if role
        statuses&.each do |status|
          filters << "status=#{status}"
        end
      end
    end

    def find_and_remove_older_siblings(path)
      [
        :find,
        Pathname.new(path).dirname.to_s,
        "-maxdepth 1",
        "-name", "'#{role_config.container_prefix}-*'",
        "!", "-name", Pathname.new(path).basename.to_s,
        "-exec rm -rf \"{}\" +"
      ]
    end

    def copy_contents(source, destination, continue_on_error: false)
      [ :cp, "-rn", "#{source}/*", destination, *("|| true" if continue_on_error)]
    end
end


Review comment (suggestion):
Looks like `-T` is GNU-only. Copying "#{source}/.", "#{destination}/." could be a workaround, avoiding the differing trailing-slash behavior in BSD vs GNU.

Code after:
      [config.service, role, config.destination].compact.join("-")
    end

    def filters(statuses: nil)
      [ "label=service=#{config.service}" ].tap do |filters|
        filters << "label=destination=#{config.destination}" if config.destination
        filters << "label=role=#{role}" if role
        statuses&.each do |status|
          filters << "status=#{status}"
        end
      end
    end

    def find_and_remove_older_siblings(path)
      [
        :find,
        Pathname.new(path).dirname.to_s,
        "-maxdepth 1",
        "-name", "'#{role_config.container_prefix}-*'",
        "!", "-name", Pathname.new(path).basename.to_s,
        "-exec rm -rf \"{}\" +"
      ]
    end

    def copy_contents(source, destination, continue_on_error: false)
      [ :cp, "-rnT", "#{source}", destination, *("|| true" if continue_on_error)]
    end
end