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] Include files from another directory for jinja2 templates #191

Closed
skylarbpayne opened this issue Mar 30, 2022 · 8 comments
Closed
Labels
enhancement New feature or request

Comments

@skylarbpayne
Copy link
Contributor

Expected Behavior

I expect deployment (or more specifically, rendering) to work with the example below.

Current Behavior

Traceback (most recent call last):
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/bin/dbx", line 8, in <module>
    sys.exit(cli())
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/dbx/commands/deploy.py", line 139, in deploy
    deployment = deployment_file_config.get_environment(environment)
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/dbx/utils/common.py", line 200, in get_environment
    return self._get_deployment_config().get(environment)
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/dbx/utils/common.py", line 192, in _get_deployment_config
    template = self._render_jinja_template()
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/dbx/utils/common.py", line 189, in _render_jinja_template
    return j2_env.get_template(file_name).render(os.environ)
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/jinja2/environment.py", line 1291, in render
    self.environment.handle_exception()
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/jinja2/environment.py", line 925, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "jobs/shared/test_job.json.j2", line 6, in top-level template code
    "new_cluster": {% include 'clusters/job_cluster.json' %},
  File "/Users/skylarbpayne/miniconda3/envs/hrdb/lib/python3.8/site-packages/jinja2/loaders.py", line 214, in get_source
    raise TemplateNotFound(template)
jinja2.exceptions.TemplateNotFound: clusters/job_cluster.json

Note: this fails with any incantation for job_cluster.json I've found. For example: relative paths from the template directory, absolute paths, etc. All result in the same problem.

If I put job_cluster.json in the same directory as example-job.json.j2 then it works. The issue is that the jinja2 environment is setup so that it only knows about the immediate parent directory of the template file; so you can only include other templates from that directory.

Making this slightly more flexible would enable much better re-use.

Steps to Reproduce (for bugs)

Two files

jobs/example-job.json.j2

{
    "default": {
        "jobs": [
            {
                "name": "example-job",
                "new_cluster": {% include 'clusters/job_cluster.json' %},
                "libraries": [],
                "max_retries": 0,
                "notebook_task": {
                    "notebook_path": "notebooks/shared/intro_to_data.py"
                },
                "max_concurrent_runs": 1,
                "email_notifications": {
                  "ON_FAILURE": ["skylar@healthrhythms.com"]
                }
            }
        ]
    }
}

clusters/job_cluster.json

{
    "spark_version": "9.1.x-cpu-ml-scala2.12",
    "node_type_id": "i3en.large",
    "spark_conf": {
        "spark.databricks.cluster.profile": "singleNode",
        "spark.master": "local[*, 4]"
    },
    "num_workers": 0,
    "custom_tags": {
        "ResourceClass": "SingleNode"
    },
    "spark_env_vars": {
        "PYSPARK_PYTHON": "/databricks/python3/bin/python3",
    },
}

Then run: dbx deploy --deployment-file jobs/example-job.json.j2

Context

See above.

Your Environment

  • dbx version used: 0.4.1
  • Databricks Runtime version: 9.1 ML

Not sure what the "best" solution is here, but I believe we could support another argument from the command line to provide additional template load paths when rendering via jinja2.

I'd be happy to take a stab at contributing this.

@skylarbpayne skylarbpayne changed the title Include files from another directory for jinja2 templates [FEATURE] Include files from another directory for jinja2 templates Mar 30, 2022
@skylarbpayne skylarbpayne changed the title [FEATURE] Include files from another directory for jinja2 templates [Feature] Include files from another directory for jinja2 templates Mar 30, 2022
@renardeinside
Copy link
Contributor

Hi @skylarbpayne ,

this feature sounds nice to me, but I'm not sure about the design. I have the following concepts in my mind:

  1. Single folder with templates:
dbx deploy --deployment-source-path=/path/to/folder

The /path/to/folder shall contain the following structure:


main.<json or yaml>.j2
reference/subpath1/to/<json or yaml>.j2
reference/subpath2/to/<json or yaml>.j2
yet-another.<json or yaml>.j2
  1. Simply resolve the references from the root of the project

Which one seems to you better? I personally prefer option #2, but only because it's way simpler to implement 😄

@skylarbpayne
Copy link
Contributor Author

skylarbpayne commented Apr 3, 2022

(2) sounds great to me! Though, I imagine you might mean "from current working directory" (which I think would be the root of the project most of the time)?

I'll sign the CLA and take a stab at this!

@skylarbpayne
Copy link
Contributor Author

My PR is out of date now -- but given lack of any response over a month, I'm not sure it's a good use of my time to update.

LMK if/when you may have some bandwidth and I'd be happy to update this. Until then, I'll move my team off dbx

@renardeinside
Copy link
Contributor

hi @skylarbpayne ,

I'm really sorry for the late reply - it's pretty hard to keep all things in sync.
I'll take your PR and make some adjustments and then merge it, also to make sure we release this as a part of 0.5.0.

@skylarbpayne
Copy link
Contributor Author

No problem! I imagine it's difficult to keep up with. If there's a communication channel that works better for you, LMK. I'm happy to help out quite a bit, but I just don't want to put the time in if there isn't bandwidth on the other end :)

Looks like there were some windows failures on this PR. I'll take a look -- I don't have a windows machine, so hoping this isn't very tricky to debug 😅

@renardeinside renardeinside added the enhancement New feature or request label May 9, 2022
@renardeinside renardeinside added this to the 0.5.0 milestone May 9, 2022
@skylarbpayne
Copy link
Contributor Author

Okay, I fixed the Windows build -- there's a codecov failure, but I think it's not a meaningful one. Seems caused by me reducing the number of lines of code in the implementation (but the method is still fully covered).

LMK if there's something more you think I should do :)

@nfx nfx removed this from the 0.5.0 milestone May 31, 2022
@renardeinside
Copy link
Contributor

hi @skylarbpayne ,

merged in #194 , will be released in 0.6.0.
Thanks a lot for reporting this issue!

@roelschr
Copy link

roelschr commented Sep 13, 2023

This has been removed at some point with refactors. @renardeinside any special reason or would you accept new PRs to re-add this feature to dbx? It seems it's just a matter of adding curdir's absolute path to the FileSystemLoader here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants