Skip to content

Conversation

@pared
Copy link
Contributor

@pared pared commented Nov 2, 2021

Fixes: #6911
Reverts: a1b1466

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

In #6550 we introduced templates in packages. While utilizing importlib.resources and including templates with our package makes sense for other types of builds, in the case of our binary packages and pyinstaller we cannot include package data, because of pyinstaller/pyinstaller#2697.

What we could do with pyinstaller would be to create resources directory alongside dvc and include the templates there. That would require us to load our templates differently depending on whether we can load data from package (other builds) or pyinstallers sys._MEIPASS when using binary packages.

This seems much more error-prone than the previous approach (keeping predefined templates as python dicts in dvc/repo/plots/template.py and is harder to test (it would require us to test binary packages).

That is why I decided to revert the change about templates in packages.

cc @dberenbaum

@pared pared requested a review from a team as a code owner November 2, 2021 12:46
@pared pared requested a review from skshetry November 2, 2021 12:46
@pared pared force-pushed the fix_package_templates_pyinstaller branch from 90cd734 to cd8dc45 Compare November 3, 2021 12:32
@pared pared added A: plots Related to the plots skip-changelog Skips changelog labels Nov 10, 2021
@pared pared force-pushed the fix_package_templates_pyinstaller branch from cd8dc45 to 4b57f30 Compare November 16, 2021 12:01
@pmrowla
Copy link
Contributor

pmrowla commented Nov 16, 2021

@pared What was the issue with adding data files in pyinstaller (it looks like you might have linked the wrong issue)? Is there a reason we can't do something like they've documented here: https://pyinstaller.readthedocs.io/en/stable/spec-files.html#adding-files-to-the-bundle

@pared
Copy link
Contributor Author

pared commented Nov 16, 2021

@pmrowla

The issue I attached is regarding the package level files - for mac and Linux we will be unable to pack the templates, simply because pyinstaller creates executable of dvc name and tries to create dvc/.../templates right after.

We could try adding the files to the bundle but that would mean that we need to use separate logic for loading templates when using a non-binary package and a different one when using binary one, which seems to me to be more error prone and would require us to test binary packages.

@pared
Copy link
Contributor Author

pared commented Nov 22, 2021

@Mergifyio rebase

@skshetry skshetry force-pushed the fix_package_templates_pyinstaller branch from 4b57f30 to 4a8ec8d Compare November 22, 2021 17:35
@mergify
Copy link
Contributor

mergify bot commented Nov 22, 2021

rebase

βœ… Branch has been successfully rebased

@pared
Copy link
Contributor Author

pared commented Nov 30, 2021

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Nov 30, 2021

rebase

βœ… Branch has been successfully rebased

@skshetry skshetry force-pushed the fix_package_templates_pyinstaller branch from 4a8ec8d to c847984 Compare November 30, 2021 12:48
@efiop
Copy link
Contributor

efiop commented Dec 6, 2021

@pared What's the status here?

@pared
Copy link
Contributor Author

pared commented Dec 6, 2021

Awaiting review

@efiop efiop merged commit 954ac6a into treeverse:master Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A: plots Related to the plots skip-changelog Skips changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

plots: loading templates from pyinstaller breaks binary builds

3 participants