-
-
Notifications
You must be signed in to change notification settings - Fork 4
Add decision filter #21
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| import subprocess | ||
| import os | ||
| import ast | ||
| import click | ||
| from .lang import Lang | ||
| from .wrangler import DIR_SCRIPT, DIR_LOG, get_universe_id_from_script, get_universe_log, get_universe_error_log, get_universe_name | ||
| from subprocess import PIPE | ||
|
|
@@ -52,3 +54,12 @@ def run_commands_in_folder(folder, file_with_commands): | |
| for line in f.readlines(): | ||
| os.system(line) | ||
| os.chdir(cwd) | ||
|
|
||
| class PythonDict(click.Option): | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add more comments / documentation? In general, we'll need one Docstring per function, ideally describing (1) what the function does, (2) any parameters, (3) the return value. You might only do (1) if the function is simple. Search for "Docstring" to learn more about the format |
||
| def type_cast_value(self, ctx, value): | ||
| try: | ||
| return dict(ast.literal_eval(value)) | ||
| except: | ||
| raise click.BadParameter(value) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,14 +18,19 @@ | |
| default='.', show_default=True) | ||
| @click.option('--lang', help='Language, can be python/R [default: inferred from file extension]', | ||
| default='') | ||
| def compile(script, out, lang): | ||
| @click.option('--decisions', '-d', cls=PythonDict, default='{}') | ||
| def compile(script, out, lang, decisions): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be more flexible if we add this "decisions" option to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And the implementation will be much simpler as we'll only need to read and filter the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the main issue with making this part of filter on filter on That was my reasoning behind choosing to use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! The example was helpful and I think the reasoning makes sense. As we discussed in the meeting, besides the review points we'll also need to combine |
||
| """Generate multiverse analysis from specifications.""" | ||
|
|
||
| check_path(script) | ||
|
|
||
| click.echo('Creating multiverse from {}'.format(script)) | ||
| ps = Parser(script, out, lang) | ||
| ps.main() | ||
|
|
||
| if decisions: | ||
| ps.main(decisions) | ||
| else: | ||
| ps.main() | ||
|
|
||
| ex = """To execute the multiverse, run the following commands: | ||
| boba run --all | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -249,14 +249,14 @@ def _code_gen_recur(self, path, i, code, history): | |
|
|
||
| if chunk.variable != '': | ||
| # check if we have already encountered the placeholder variable | ||
| prev_idx = None | ||
| prev_option = None | ||
| for d in history.decisions: | ||
| if d.parameter == chunk.variable: | ||
| prev_idx = d.idx | ||
| prev_option = d.option | ||
|
|
||
| if prev_idx is not None: | ||
| if prev_option is not None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does |
||
| # use the previous value | ||
| snippet, opt = self.dec_parser.gen_code(chunk.code, chunk.variable, prev_idx) | ||
| snippet, opt = self.dec_parser.gen_code(chunk.code, option=prev_option) | ||
| self._code_gen_recur(path, i+1, code+snippet, history) | ||
| else: | ||
| # expand the decision | ||
|
|
@@ -272,7 +272,7 @@ def _code_gen_recur(self, path, i, code, history): | |
| continue | ||
|
|
||
| # code gen | ||
| snippet, opt = self.dec_parser.gen_code(chunk.code, chunk.variable, k) | ||
| snippet, opt = self.dec_parser.gen_code(chunk.code, dec_id=chunk.variable, i_alt=k) | ||
| decs = [a for a in history.decisions] | ||
| decs.append(DecRecord(chunk.variable, opt, k)) | ||
| self._code_gen_recur(path, i+1, code + snippet, | ||
|
|
@@ -281,7 +281,7 @@ def _code_gen_recur(self, path, i, code, history): | |
| code += chunk.code | ||
| self._code_gen_recur(path, i+1, code, history) | ||
|
|
||
| def _code_gen(self): | ||
| def _code_gen(self, decrecords=None, path_filter=None): | ||
| paths = self._get_code_paths() | ||
|
|
||
| self.wrangler.counter = 0 # keep track of file name | ||
|
|
@@ -290,8 +290,46 @@ def _code_gen(self): | |
| + len(self.code_parser.get_decisions()) | ||
|
|
||
| self.wrangler.create_dir() | ||
| for idx, p in enumerate(paths): | ||
| self._code_gen_recur(p, 0, '', History(idx)) | ||
|
|
||
| if decrecords is None: | ||
| for idx, p in enumerate(paths): | ||
| self._code_gen_recur(p, 0, '', History(idx)) | ||
| elif path_filter is None: | ||
| for idx, p in enumerate(paths): | ||
| self._code_gen_recur(p, 0, '', History(idx, '', decrecords, [])) | ||
| else: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly you might want to encapsulate this chunk of code in a function that returns |
||
| valid_paths = [] | ||
| for idx, p in enumerate(paths): | ||
| run_path = True | ||
| filtered_nodes = set() | ||
| for node in p: | ||
| if ':' in node[0]: | ||
| split = node[0].split(':') | ||
| name = split[0] | ||
| value = split[1] | ||
|
|
||
| if name not in path_filter: | ||
| continue | ||
|
|
||
| if path_filter[name] != value: | ||
| run_path = False | ||
| break | ||
| else: | ||
| filtered_nodes.add(name) | ||
|
|
||
| for node in path_filter: | ||
| if node not in filtered_nodes: | ||
| run_path = False | ||
| break | ||
|
|
||
| if run_path: | ||
| valid_paths.append((idx, p)) | ||
|
|
||
| if not valid_paths: | ||
| raise ValueError('bad input to path_filter!') | ||
|
|
||
| for path in valid_paths: | ||
| self._code_gen_recur(path[1], 0, '', History(path[0], '', decrecords, [])) | ||
|
|
||
| # write the pre and post execs to a file. | ||
| self.wrangler.write_pre_exe() | ||
|
|
@@ -387,9 +425,37 @@ def _warn_size(self): | |
| print('Aborted.') | ||
| exit(0) | ||
|
|
||
| def main(self, verbose=True): | ||
| def main(self, decisions=None, verbose=True): | ||
| self._warn_size() | ||
| self._code_gen() | ||
|
|
||
| if decisions is not None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps move this chunk of code to a new function in the parser class (or a function in the decision parser class). We want to keep this |
||
| decrecords = [] | ||
| code_path = 'path_filter' | ||
| if code_path in decisions: | ||
| path_filter = decisions[code_path] | ||
| else: | ||
| path_filter = None | ||
|
|
||
| for key, value in decisions.items(): | ||
| if key == code_path: | ||
| continue | ||
|
|
||
| dec_exist = False | ||
| for decname, dec in self.dec_parser.decisions.items(): | ||
| if decname == key: | ||
| if not dec.check_value(value): | ||
| raise ValueError('The decision "' + key + '" cannot have the value "' + str(value) + '"') | ||
| dec_exist = True | ||
|
|
||
| if not dec_exist: | ||
| raise ValueError('The decision "' + key + '" does not exist') | ||
|
|
||
| decrecords.append(DecRecord(key, value, 0)) | ||
|
|
||
| self._code_gen(decrecords, path_filter) | ||
| else: | ||
| self._code_gen() | ||
|
|
||
| self._write_csv() | ||
| self._write_server_config() | ||
| if verbose: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,7 @@ | |
| "seed" : 0, | ||
| "sample" : "uniform", | ||
| "count" : 50, | ||
| "min" : 1.0, | ||
| "max" : 3.0 | ||
| "range" : [1.0, 3.0] | ||
| } | ||
| ] | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the class is a bit too general. Maybe indicating it's a custom click command class. Also why not move it to
cli.py(if it's only used there? I might be wrong)