-
Notifications
You must be signed in to change notification settings - Fork 9
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
added ability to allow execution dag to be submitted and validated st… #96
Conversation
execution_dag_schema = json.load(f) | ||
|
||
|
||
def validate_schema(query): |
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.
Rename function to: prevalidate
or similar -- something to indicate this is just a cursory validation.
Doc string along the lines of:
This performs a cursory validation to see if we have the basic format of an execution DAG. Note that this does not do any in-depth checks to see if [conforms to functions we have, security/injection, correct parameters, etc.], but just [what it does do]. [Prevent basic errors]
query = dag | ||
if not learning_observer.communication_protocol.schema.validate_schema(query): | ||
debug_log(f'Submitted execution query is not a valid form') | ||
funcs.append(DAGIncorrectFormat()) |
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.
I would consider including the validation exception in the DAGIncorrectFormat
, so we can provide some useful debugging info to the client.
return True | ||
except jsonschema.ValidationError as e: | ||
print(e) | ||
return False |
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.
We might want to bubble the specific error back to the client in some way. I might consider continuing to raise an exception to be handled downstream. Perhaps, in pseudocode, something like: raise DAGIncorrectFormat(ValidationError.string_format())
debug_log(f'Execution DAG, {dag_name}, not found.') | ||
funcs.append(DAGNotFound(dag_name)) | ||
dag = client_query.get('execution_dag', query_name) | ||
if learning_observer.settings.settings.get('dangerously_allow_insecure_dags', False) and type(dag) == dict: |
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.
if type is string:
do stringy_stuff
if type is dict:
if not dangerously_allow_dags:
raise security_exception
do_dicty_stuff
else:
bad type
Or possibly:
def do_stringy_stuff()
pass
def do_dicty_stuff()
pass
dispatch = {dict: do_dicty_stuff, str: do_stringy_stuff}
if type(dag) not in dispatch: raise exception
dispatch[type(dag)]
* Working styling * LOStudentTable * Sample essays can now return essays of multiple types * PEP8 * Minor bug fix handling INF * Sync between devices * Working styling * LOStudentTable * Sample essays can now return essays of multiple types * PEP8 * Minor bug fix handling INF * Sync between devices * added js2py and fixed css file reference --------- Co-authored-by: Piotr Mitros <piotr@mitros.org>
Allow execution dags to include 1 level of dependencies. For example, the GPT dashboard now uses a dag within its module that references the Writing Observer dag.
…g_observer into comm-protocol-schema
Github is doing the weird pull request thing again. I will close this and reopen. |
…ructure