Add active_encode support to hydra-derivatives #161

cjcolvar · 2017-05-10T15:51:53Z

The ActiveEncode runner allows using ActiveEncode within hydra-derivatives to generate derivatives using the adapters supported by ActiveEncode. Currently Amazon Elastic Transcoder is the only production ready adapter available but prototypes exist for zencoder and shingoncoder (FFmpeg through ActiveJob). The key differences between ActiveEncode and hydra-derivatives are:

ActiveEncode can produce multiple derivatives of potentially different formats (video derivatives, thumbnail images) from a single invocation
ActiveEncode does not necessarily operate on local files
ActiveEncode returns a url to the derivatives it creates and not a bitstream
ActiveEncode allows for polling to get updates on its progress

In support of this a source file service and output file service have been added. The RemoteServiceFile takes the :source option and calls a method of that name on the source object to determine the source file path or url to send to the processor. The PersistExternalFileOutputService creates an ActiveFedora::File with a url to the derivative output instead of storing the actual bit contents of the derivative in Fedora.

Documentation is added for configuring and using the ActiveEncode runner with Amazon Elastic Transcoder which seems like the main use case for users at this point.

Thanks to @val99erie for a lot of this work.

transcoders besides ffmpeg. Story avalonmediasystem/avalon#1785

carolyncole

@cjcolvar the PR looks good. I added a few comments on code style. I have done little with AWS so I am not inclined to approve or disapprove...

carolyncole · 2017-05-10T16:23:45Z

lib/hydra/derivatives/processors/active_encode.rb

+module Hydra::Derivatives::Processors
+  class ActiveEncodeError < StandardError
+    def initialize(status, source_path, errors = [])
+      msg = "ActiveEncode status was \"#{status}\" for #{source_path}"


@cjcolvar Could this be I18N?

There currently isn't any i18n in this gem and I haven't found any examples of non-UI strings using i18n in hydra gems. Do you think it is still worth doing?

i18nizing hydra-derivatives from the ground up feels like a separate stream of work.

carolyncole · 2017-05-10T16:25:25Z

lib/hydra/derivatives/processors/active_encode.rb

+      end
+    end
+
+    def wait_for_encode_with_timeout(encode)


Should this method be part of the public interface or should it and the methods below be private?

👍 Makes sense to me.

carolyncole · 2017-05-10T16:35:37Z

lib/hydra/derivatives/processors/active_encode.rb

+    # After a timeout error, try to cancel the encoding.
+    def cleanup_after_timeout(encode)
+      encode.cancel!
+    rescue => e


It would be safer to rescue a typed error here

At this point it isn't really defined what errors may be thrown. Seems like what might need to happen is ActiveEncode's adapters should catch any errors thrown and raise standard errors defined by ActiveEncode. I believe the default type for rescue is StandardError and I think that is the best that can be done right now.

carolyncole · 2017-05-10T16:36:36Z

lib/hydra/derivatives/runners/active_encode_derivatives.rb

+      end
+    end
+
+    def self.processor_options(options)


Should this method and the ones below be public?

.processor_options is the only method here that isn't an override of a public method in Runner. I'll move it to be private.

cjcolvar · 2017-05-11T13:28:44Z

@Cam156 Thanks for your comments. I think I responded or handled them in the last commit I pushed.
@mjgiarlo Do you mind reviewing as well since you looked over things while they were WIP?

mjgiarlo · 2017-05-11T14:18:44Z

@cjcolvar You bet.

mjgiarlo · 2017-05-11T14:45:47Z

README.md

@@ -74,6 +74,7 @@ Hydra::Derivatives::Processors::Video::Processor.timeout  = 10.minutes
 Hydra::Derivatives::Processors::Document.timeout = 5.minutes
 Hydra::Derivatives::Processors::Audio.timeout = 10.minutes
 Hydra::Derivatives::Processors::Image.timeout = 5.minutes
+Hydra::Derivatives::Processors::ActiveEncode.timeout = 5.minutes


Perhaps a naïve question: if I have video files, how do I know whether to use the Video processor or the ActiveEncode processor? How would you feel about adding a short section to the README immediately above ## Configuration that makes clear what the options are for generating derivatives from video? I.e., answering the question, "what does ActiveEncode support bring to hydra-derivatives, and why/when would I use it?"

mjgiarlo · 2017-05-11T14:48:05Z

hydra-derivatives.gemspec

@@ -27,6 +27,7 @@ Gem::Specification.new do |spec|
  spec.add_dependency 'mini_magick', '>= 3.2', '< 5'
  spec.add_dependency 'activesupport', '>= 4.0', '< 6'
  spec.add_dependency 'mime-types', '> 2.0', '< 4.0'
+  spec.add_dependency 'active_encode'


Should this pin to '~> 0.1', or put otherwise: what's the roadmap look like for AE, and might changes come along in 1.0.0 that would break hydra-derivatives?

mjgiarlo · 2017-05-11T14:50:52Z

lib/hydra/derivatives.rb

@@ -8,9 +8,12 @@ module Derivatives
    extend Deprecation
    self.deprecation_horizon = "hydra-derivatives 1.0"

+    require 'active_encode'


Do we need this require here? Other dependencies, like mini_magick, are required where needed rather than in the Derivatives module.

mjgiarlo · 2017-05-11T14:52:23Z

lib/hydra/derivatives/processors/active_encode.rb

+module Hydra::Derivatives::Processors
+  class ActiveEncodeError < StandardError
+    def initialize(status, source_path, errors = [])
+      msg = "ActiveEncode status was \"#{status}\" for #{source_path}"


i18nizing hydra-derivatives from the ground up feels like a separate stream of work.

mjgiarlo · 2017-05-11T14:57:27Z

lib/hydra/derivatives/processors/active_encode.rb

+  class ActiveEncodeError < StandardError
+    def initialize(status, source_path, errors = [])
+      msg = "ActiveEncode status was \"#{status}\" for #{source_path}"
+      msg = "#{msg}: #{errors.join(' ; ')}" unless errors.empty?


Can you use if errors.any? here instead of unless errors.empty??

mjgiarlo · 2017-05-11T15:10:57Z

lib/hydra/derivatives/runners/active_encode_derivatives.rb

+    class << self
+      private
+
+        def processor_options(options)


What is the purpose of this method? It looks like it's taking one hash and building a new, similar one from it. I get that much, but why's it needed? (Pls add a comment above this method to make that clear.)

mjgiarlo · 2017-05-11T15:19:23Z

lib/hydra/derivatives/services/persist_external_file_output_file_service.rb

+      external_file.content = ''
+      external_file.mime_type = "message/external-body; access-type=URL; URL=\"#{output[:url]}\""
+      # external_file.external_url = output[:url]
+      external_file.original_name = URI.parse(output[:url]).path.split('/').last


If you use Addressable::URI.parse here, it would be more unicode-friendly.

mjgiarlo · 2017-05-11T15:20:07Z

lib/hydra/derivatives/services/remote_source_file.rb

+module Hydra::Derivatives
+  class RemoteSourceFile
+    # Finds the file name of the remote source file.
+    # @param [String, ActiveFedora::Base] String file name, or an object that has a method that will return the file name


I believe the second String here should be object, the name of the param.

mjgiarlo · 2017-05-11T15:20:56Z

spec/processors/active_encode_spec.rb

+require 'spec_helper'
+
+describe Hydra::Derivatives::Processors::ActiveEncode do
+  # before { # ActiveEncode::Base.engine_adapter = :test }


mjgiarlo · 2017-05-11T15:23:14Z

spec/processors/active_encode_spec.rb

+      end
+
+      it 'processes the encoding without a timeout' do
+        expect(processor).to receive(:wait_for_encode_with_timeout).never


What would you think of using .not_to receive here?

cjcolvar · 2017-05-15T18:46:37Z

@mjgiarlo I think I took care of everything that isn't documentation or comments changes. I'll come back to those later today/tomorrow.

mjgiarlo · 2017-05-15T19:37:04Z

@cjcolvar OK, thanks! Ping me when it's 💚 and all ready to go.

…n it should be used.

coveralls · 2017-06-09T13:55:46Z

Changes Unknown when pulling ee0ad3f on active_encode_dev_con into ** on master**.

coveralls · 2017-06-09T13:55:46Z

Changes Unknown when pulling ee0ad3f on active_encode_dev_con into ** on master**.

coveralls · 2017-06-09T13:55:46Z

Changes Unknown when pulling ee0ad3f on active_encode_dev_con into ** on master**.

cjcolvar · 2017-06-09T14:03:33Z

@mjgiarlo Ready. Any additional changes you'd like to see?

mjgiarlo · 2017-06-09T15:14:47Z

@cjcolvar Taking one last look. Thanks for all the changes, and yr patience!

mjgiarlo · 2017-06-09T15:17:01Z

🎉 Thanks for the contribution! Great work. (Do you want/need a new release now that this is merged?)

Add active_encode gem to hydra-derivatives so we can use other

8a8d045

transcoders besides ffmpeg. Story avalonmediasystem/avalon#1785

carolyncole reviewed May 10, 2017

View reviewed changes

Privitize methods

68b0a04

Remove Rubocop exclusions and violations

e68faca

mjgiarlo force-pushed the active_encode_dev_con branch from 24bc216 to e68faca Compare May 11, 2017 14:41

mjgiarlo requested changes May 11, 2017

View reviewed changes

cjcolvar mentioned this pull request May 12, 2017

PR into Hydra Derivatives avalonmediasystem/avalon#1827

Closed

4 tasks

cjcolvar added 2 commits May 15, 2017 14:45

Refactor AE processor to use attr for encode job instance

807fc3f

Use Addressable to make uri parsing more awesomer

5a9c69b

cjcolvar added 3 commits May 16, 2017 21:59

Fix tests

7d3ab98

Add note to README explaining what the ActiveEncode runner is and whe…

e97f6e9

…n it should be used.

Explain purpose of processor_options method

ee0ad3f

mjgiarlo approved these changes Jun 9, 2017

View reviewed changes

mjgiarlo merged commit 6e68c3d into master Jun 9, 2017

mjgiarlo deleted the active_encode_dev_con branch June 9, 2017 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add active_encode support to hydra-derivatives #161

Add active_encode support to hydra-derivatives #161

cjcolvar commented May 10, 2017

carolyncole left a comment

carolyncole May 10, 2017

cjcolvar May 10, 2017

mjgiarlo May 11, 2017

carolyncole May 10, 2017

cjcolvar May 10, 2017

carolyncole May 10, 2017

cjcolvar May 10, 2017

carolyncole May 10, 2017

cjcolvar May 10, 2017

cjcolvar commented May 11, 2017

mjgiarlo commented May 11, 2017

mjgiarlo May 11, 2017

mjgiarlo May 11, 2017

mjgiarlo May 11, 2017

mjgiarlo May 11, 2017

mjgiarlo May 11, 2017

mjgiarlo May 11, 2017

mjgiarlo May 11, 2017

mjgiarlo May 11, 2017

mjgiarlo May 11, 2017

mjgiarlo May 11, 2017

cjcolvar commented May 15, 2017

mjgiarlo commented May 15, 2017

coveralls commented Jun 9, 2017

coveralls commented Jun 9, 2017

coveralls commented Jun 9, 2017

cjcolvar commented Jun 9, 2017

mjgiarlo commented Jun 9, 2017

mjgiarlo commented Jun 9, 2017 •

edited

Add active_encode support to hydra-derivatives #161

Add active_encode support to hydra-derivatives #161

Conversation

cjcolvar commented May 10, 2017

carolyncole 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

cjcolvar commented May 11, 2017

mjgiarlo commented May 11, 2017

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

cjcolvar commented May 15, 2017

mjgiarlo commented May 15, 2017

coveralls commented Jun 9, 2017

coveralls commented Jun 9, 2017

coveralls commented Jun 9, 2017

cjcolvar commented Jun 9, 2017

mjgiarlo commented Jun 9, 2017

mjgiarlo commented Jun 9, 2017 • edited

mjgiarlo commented Jun 9, 2017 •

edited