Feature: categorization of notebooks to simplify huge repositories navigation #190
Conversation
|
Doesn't affect original notebooker navigation if categorization flag is not set |
|
Checks pipeline is not running: |
|
@jonbannister can you take a look? |
|
@jonbannister Hi! Please review |
There was a problem hiding this comment.
I think this needs a different approach. We cannot rely on current_app.config to store the PATH_TO_CATEGORY_DICT, and we can't rely on using the templates to display results - e.g. what if the templates completely change: do we lose all the results?
The categories should be a first-class piece of metadata associated with the report_results and, if activated in the webapp, should essentially act like a top level folder - possibly with different endpoints to make it clearer/cleaner. We shouldn't have lots of if statements everywhere which slightly change behaviour, it should follow a (minimal) separate code path to do the right thing.
I imagine this could be done as a thin layer on top of the existing folder structure which just adds another layer of depth to the frontpage.
There was a problem hiding this comment.
Could we please revert the version bump until we have ensured the tests pass on master
There was a problem hiding this comment.
Could we please add some documentation/screenshots of the functionality?
| "--categorization", | ||
| default=False, | ||
| is_flag=True, | ||
| help="If selected, discovers only templates with the 'category=example' tags set to any cell and groups notebooks by their category names", |
There was a problem hiding this comment.
What is the expected behaviour when multiple cells in the same notebook have clashing categories?
| @@ -180,6 +188,7 @@ def start_webapp( | |||
|
|
|||
| @base_notebooker.command() | |||
| @click.option("--report-name", help="The name of the template to execute, relative to the template directory.") | |||
| @click.option("--category", default="", help="Category of the template.") | |||
There was a problem hiding this comment.
Doesn't this come from the cell tags? How come this needs to be passed in?
| @@ -351,6 +357,7 @@ def _get_overrides(overrides_as_json: AnyStr, iterate_override_values_of: Option | |||
| def execute_notebook_entrypoint( | |||
| config: BaseConfig, | |||
| report_name: str, | |||
| category: str, | |||
There was a problem hiding this comment.
This is an Optional[str]
| return new_dict | ||
|
|
||
| filtered_dict = filter_dict(d) | ||
| return strip_extensions(filtered_dict) |
There was a problem hiding this comment.
How come all of this extra filtering and cleaning is required? It wasn't beforehand, and the categories do not add new files to clutter the output structure as far as I understand.
|
|
||
| def get_all_templates(): | ||
| if current_app.config["CATEGORIZATION"]: | ||
| return get_all_possible_templates(warn_on_local=False) |
There was a problem hiding this comment.
This changes the output type - is that intended?
| @@ -14,7 +14,7 @@ def test_create_schedule(flask_app, setup_workspace): | |||
| rv = client.get("/core/all_possible_templates_flattened") | |||
| assert rv.status_code == 200 | |||
| data = json.loads(rv.data) | |||
| assert data == {"result": ["fake/py_report", "fake/ipynb_report", "fake/report_failing"]} | |||
| assert sorted(data["result"]) == sorted(["fake/py_report", "fake/ipynb_report", "fake/report_failing"]) | |||
There was a problem hiding this comment.
Why did the ordering change?
| // title: "Report Name", | ||
| // name: "report_name", | ||
| // data: "params.report_name", | ||
| // }, |
There was a problem hiding this comment.
Why comment out the report name?
| @@ -52,6 +52,8 @@ def remove_schedule(job_id): | |||
|
|
|||
|
|
|||
| def get_job_id(report_name: str, report_title: str) -> str: | |||
| if "PATH_TO_CATEGORY_DICT" in current_app.config and report_name in current_app.config["PATH_TO_CATEGORY_DICT"]: | |||
There was a problem hiding this comment.
I don't think this way of retrieving the category is tenable - it assumes the webapp still has access to the template files (which is not guaranteed) and that get_directory_structure() has run (which is not guaranteed at this point as far as I know)
0.6.4 (2024-10-08)