Skip to content

Replace deprecated appdirs with platformdirs, fix config path lookup with non conda-based environments #113

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

Merged

Conversation

agriyakhetarpal
Copy link
Contributor

@agriyakhetarpal agriyakhetarpal commented Jan 24, 2025

Description

As the appdirs project is no longer maintained (ActiveState/appdirs@8734277), this pull request swaps out the dependency to use platformdirs. Additionally, it changes empack's DEFAULT_CONFIG_PATH lookup to also cater to virtual environments that are not managed by conda or equivalent tools, in light of matplotlib/matplotlib#29506 where a problem was noticed when using pyenv-based Python installations/interpreters.

Additional context

@agriyakhetarpal
Copy link
Contributor Author

Should I add a CHANGELOG entry as well here, or will it be picked up by the PR title automatically? It would be nice to get this PR in a release, but there's no hurry :D

Copy link
Contributor

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add a CHANGELOG entry as well here, or will it be picked up by the PR title automatically?

It will be picked up automatically :) Happy to make a patch release with this! Thanks!

@@ -24,7 +24,7 @@ classifiers = [
"Programming Language :: Python :: 3.11",
]
dependencies = [
"appdirs",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the CI says appdirs is still being used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be addressed by emscripten-forge/pyjs-code-runner#15, marking for completeness :)

Copy link
Contributor

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I believe the change is not correct. It does not respect the current conda environment and it installs at the root config dir: '/home/martin/.config/empack' for me, while it should respect that I'm using micromamba here so /home/martin/micromamba/envs/whatever/share/empack or /home/martin/micromamba/envs/whatever/.config/empack

@agriyakhetarpal
Copy link
Contributor Author

@agriyakhetarpal
Copy link
Contributor Author

agriyakhetarpal commented Jan 24, 2025

Actually I believe the change is not correct. It does not respect the current conda environment and it installs at the root config dir: '/home/martin/.config/empack' for me, while it should respect that I'm using micromamba here so /home/martin/micromamba/envs/whatever/share/empack or /home/martin/micromamba/envs/whatever/.config/empack

This would be by design of platformdirs. If we want DEFAULT_CONFIG_PATH to be different per installation/per micromamba environment instead of a global one, we would need to set it to os.environ["MAMBA_ROOT_PREFIX"] / "share". Either way, sys.prefix isn't a nice idea because it varies, as we have noticed. What do you think?

@martinRenou
Copy link
Contributor

I'm surprised that sys.prefix isn't set properly when using pyenv, isn't it using the right Python?

@agriyakhetarpal
Copy link
Contributor Author

agriyakhetarpal commented Jan 24, 2025

I'm surprised that sys.prefix isn't set properly when using pyenv, isn't it using the right Python?

I'm surprised, too, as I just installed pyenv for the first time and it's the same as Python elsewhere (from Homebrew/conda/etc.):

'/opt/homebrew/opt/python@3.10/Frameworks/Python.framework/Versions/3.10

Something else must be at play.

@agriyakhetarpal
Copy link
Contributor Author

As we want to ensure that a config file must be tied to the current installation of empack and hence to a specific conda/mamba/micromamba/venv env, is to store the config file at empack.__file__, next to __init__.py, which would always be consistent.

@agriyakhetarpal
Copy link
Contributor Author

I hope 8739917 is acceptable! I tried it out locally, and it passes the test suite. With this commit, the empack_config.yaml file will be checked for in this location:

```/home/martin/micromamba/envs/whatever/lib/python3.X/site-packages/empack/empack_config.yaml`

which, I feel, is better than having to deal with sys.prefix, or $CONDA_PREFIX/share, or $MAMBA_PREFIX/share separately, because it's possible to use empack with venvs, outside of micromamba. I think we should be good to go after emscripten-forge/pyjs-code-runner#15 is merged, as it is pulled from Git.

pyproject.toml Outdated
@@ -48,9 +48,6 @@ empack = "empack.cli.main:app"
[project.urls]
Homepage = "https://github.com/emscripten-forge/empack"

[tool.hatch.build.targets.wheel.shared-data]
config = "share/empack"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I won't be too annoying to you on this. I have a preference for this to stay 🫤

I believe we should make sure we can find this path when using pyenv.

It seems jupyter has something like that https://github.com/jupyter/jupyter_core/blob/main/jupyter_core/paths.py#L102 for finding whether or not it should use user-level paths or conda environment paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope I won't be too annoying to you on this.

Absolutely not! :D I'd also love to ensure pyenv works and not move anything else. Let me port Jupyter's thing here, then.

@agriyakhetarpal
Copy link
Contributor Author

Okay, so, based on that assessment – this means that usage of empack has been broken outside conda/mamba/micromamba, as pyenv doesn't do anything special – any virtual environment with virtualenv/venv/uv would face or have faced this problem, and that explains why we faced this in CircleCI.

Could you check 273966b? I've ported over Jupyter's logic, but I don't know if the license notes would be needed. This seems to resolve the issue locally:

>>> from empack.pack import DEFAULT_CONFIG_PATH
>>> DEFAULT_CONFIG_PATH

now gets us

PosixPath('/Users/agriyakhetarpal/.pyenv/versions/3.10.4/share/empack/empack_config.yaml')

while retaining the behaviour required by micromamba, as you suggested, which is at

PosixPath('/opt/homebrew/Caskroom/miniforge/base/share/empack/empack_config.yaml'

@martinRenou martinRenou added the bug Something isn't working label Jan 24, 2025
@agriyakhetarpal agriyakhetarpal changed the title Replace deprecated appdirs with platformdirs, use platformdirs.user_config_dir for DEFAULT_CONFIG_PATH Replace deprecated appdirs with platformdirs, fix config path with non conda-based environments Jan 24, 2025
@agriyakhetarpal agriyakhetarpal changed the title Replace deprecated appdirs with platformdirs, fix config path with non conda-based environments Replace deprecated appdirs with platformdirs, fix config path lookup with non conda-based environments Jan 24, 2025
@martinRenou
Copy link
Contributor

@agriyakhetarpal it seems you need to run black on the Python code to lint it.

Also how about we install appdirs on the CI, just to get it running for now, not waiting for a pyjs update?

@agriyakhetarpal
Copy link
Contributor Author

Thanks! Addressed both points.

@martinRenou
Copy link
Contributor

Sorry, it seems there is a last linting issue 😓

@agriyakhetarpal
Copy link
Contributor Author

Argh, strange that it didn't happen locally 😅 Addressed with 3b3a32e.

Copy link
Contributor

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@martinRenou martinRenou merged commit e447e23 into emscripten-forge:main Jan 27, 2025
4 checks passed
@agriyakhetarpal agriyakhetarpal deleted the fix/appdirs/platformdirs branch January 27, 2025 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants