Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/optional files #181

Closed
36 changes: 22 additions & 14 deletions src/drake/core.clj
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,21 @@
(assoc step :inputs branch-adjusted-inputs
:outputs branch-adjusted-outputs)))

(defn- normalize-filename-for-run
(defn- normalize-filename-for-run*
"Normalizes filename and also removes local filesystem prefix (file:) from
it. This is safe to do since it's the default filesystem,
but it gives us a bit easier compatibility with existing tools."
[filename]
(let [n (dfs/normalized-path filename)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chen-factual What do you think about a function that wraps functions so that they correctly handle optional inputs? Like instead of making normalize-filename-for-run and similar functions call parser/modify-filename themselves, you could just rename the old definition to normalize-filename-for-run*, and then (def normalize-filename-for-run (wrap-optional-handling normalize-filename-for-run*)) or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a pretty awesome idea. I'll go ahead and pop that in there when I make my modifications for your review comments.

I'll probably do this late night tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, I thought about this some more. Why would you ever NOT want a function to correctly handle optional files? Why even have a normalize-filename-for-run* that fails for optional files?

Also in the case of functions like parser/add-prefix there there are other arguments besides a file name, this wrapping breaks down as we have no way of telling which argument is the file name.

I'm open to improving this in another way if you have any other ideas though.

(if (= "file" (dfs/path-fs n)) (dfs/path-filename n) n)))
(if (= "file" (dfs/path-fs n))
(dfs/path-filename n)
n)))

(defn- normalize-filename-for-run
[filename]
(parser/modify-filename
filename
normalize-filename-for-run*))

(defn- despace-cmds
"Given a sequence of commands, removes leading whitespace found in the first
Expand Down Expand Up @@ -144,7 +152,7 @@
normalized-outputs (map normalize-filename-for-run outputs)
normalized-inputs (map normalize-filename-for-run inputs)
vars (merge vars
(parser/inouts-map normalized-inputs "INPUT")
(parser/existing-inputs-map normalized-inputs "INPUT")
(parser/inouts-map normalized-outputs "OUTPUT"))
method (methods (:method opts))
method-mode (:method-mode opts)
Expand Down Expand Up @@ -176,6 +184,16 @@
:vars vars
:opts (if-not method opts (merge (:opts method) opts)))))

(defn- existing-and-empty-inputs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should return a map like {:existing (...) :empty (...)}, so that it will be clearer at the usage site what's going on.

"Remove '?' denoting optional file from front of path"
[inputs]
(let [inputs-info (map parser/make-file-stats inputs)]
{:existing (->> (filter :exists inputs-info)
(map :file))
;; Non-existing, non-optional inputs
:missing (->> (remove (some-fn :optional :exists) inputs-info)
(map :file))}))

(defn- should-build?
"Given the parse tree and a step index, determines whether it should
be built and returns the reason (e.g. 'timestamped') or
Expand All @@ -195,7 +213,7 @@
[step forced triggered match-type fail-on-empty]
(trace "should-build? fail-on-empty: " fail-on-empty)
(let [{:keys [inputs outputs opts]} (branch-adjust-step step false)
empty-inputs (filter #(not (fs di/data-in? %)) inputs)
{inputs :inputs empty-inputs :missing} (existing-and-empty-inputs inputs)
no-outputs (empty? outputs)]
(trace "should-build? forced:" forced)
(trace "should-build? match-type:" match-type)
Expand Down Expand Up @@ -320,16 +338,6 @@
true if the step was actually run; false if skipped."
[parse-tree step-number {:keys [index build match-type opts]}]
(let [{:keys [inputs] :as step} (get-in parse-tree [:steps index])]
;; TODO(artem)
;; Somewhere here, before running the step or checking timestamps, we need to
;; check for optional files and replace them with empty strings if they're
;; not found (according to the spec). We shouldn't just rewrite :inputs and
;; should probably separate two versions, since the step name
;; (used in debugging and log files names) should not vary.
;; For now just check that none of the input files is optional.
(if (some #(= \? (first %)) inputs)
(throw+ {:msg (str "optional input files are not supported yet: "
inputs)}))
(let [step-descr (step-string (branch-adjust-step step false))
step (-> step
(update-in [:opts] merge opts)
Expand Down
2 changes: 1 addition & 1 deletion src/drake/fs.clj
Original file line number Diff line number Diff line change
Expand Up @@ -464,4 +464,4 @@

(defn newest-in
[path]
(pick-by-mod-time path -))
(pick-by-mod-time path -))
57 changes: 56 additions & 1 deletion src/drake/parser.clj
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
[clojure.string :as s]
[clojure.core.memoize :as memo]
[slingshot.slingshot :refer [throw+]]
[drake.fs :as dfs]
[drake.shell :refer [shell]]
[drake.steps :refer [add-dependencies calc-step-dirs]]
[drake.utils :as utils :refer [clip ensure-final-newline]]
[drake.parser_utils :refer :all]
[drake-interface.core :as di]
[name.choi.joshua.fnparse :as p]
[fs.core :as fs]))

Expand Down Expand Up @@ -297,14 +299,49 @@
second))] ;; first is ",", second is <file-name>
(cons first-file rest-files)))

(defn add-prefix
(def ^:private ^:const opt-flag \?)

(defn optional-input?
"Check if the first character of a file specification is '?',
indicating it is an optional input"
[input]
(= opt-flag (first input)))

(defn normalize-optional-file
[filename]
(if (optional-input? filename)
(subs filename 1)
filename))

(defn make-file-stats
[filename]
(let [filepath (normalize-optional-file filename)]
{:optional (optional-input? filename)
:exists (dfs/fs di/data-in? filepath)
:path filepath
:file filename}))

(defn modify-filename
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this function. A good solution to the problem.

[filename mod-fn]
(if (optional-input? filename)
(str opt-flag (mod-fn (normalize-optional-file filename)))
(mod-fn filename)))

(defn- add-prefix*
"Appends prefix if necessary (unless prepended by '!')."
[prefix file]
(cond (= \! (first file)) (clip file)
(= \/ (first file)) file
(re-matches #"[a-zA-z][a-zA-Z0-9+.-]*:.*" file) file
:else (str prefix file)))

(defn add-prefix
"add-prefix*, with support for file naming conventions"
[prefix file]
(modify-filename
file
(fn [f] (add-prefix* prefix f))))

(defn add-path-sep-suffix [^String path]
(if (or (empty? path)
(.endsWith path "/")
Expand Down Expand Up @@ -339,6 +376,24 @@
(for [[i v] (map-indexed vector items)]
[(str prefix i) v])))

(defn- fname->var
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using empty strings here?

"Convert a file name to a var if it exists. If it
does not exist, and is not optional, use empty
string as var representation"
[filename]
(let [file-stats (make-file-stats filename)]
(if (and (not (:exists file-stats))
(:optional file-stats))
""
(:path file-stats))))

(defn existing-inputs-map
"Like inouts-map, except optional but nonexisting
input files are replaced by empty strings"
[items prefix]
(let [items (map fname->var items)]
(inouts-map items prefix)))

;; TODO(artem)
;; Should we move this to a common library?
(defn demix
Expand Down
2 changes: 1 addition & 1 deletion src/drake/parser_utils.clj
Original file line number Diff line number Diff line change
Expand Up @@ -240,4 +240,4 @@
(p/complex [_ single-quote
chars (p/rep* (p/except p/anything single-quote))
_ single-quote]
(apply str chars)))
(apply str chars)))