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

Refactor user data directories with platformdirs #122

Merged
merged 9 commits into from
Oct 10, 2023

Conversation

cmutel
Copy link
Collaborator

@cmutel cmutel commented Oct 10, 2023

Fixes #100.

The current implementation stores user data in the site-packages directory. This is undesired, as:

  • Users should not touch this directory in general
  • It makes multi-user installations problematic for many reasons

The alternative is to give each user a cache directory in a suitable location. We follow other Brightway libraries and user platformdirs, the successor to appdirs.

This PR also centralized the location of directory path creation, which should prevent inconsistencies, and provides one clear place where one can find filesystem constants: filesystem_constants.py.

@cmutel cmutel requested a review from tngTUDOR October 10, 2023 13:31
@cmutel
Copy link
Collaborator Author

cmutel commented Oct 10, 2023

@romainsacchi @tngTUDOR I would love to get your input on the following:

The current export directory structure could also be refactored, so that the export directory wouldn't depend on the current working directory. Are you open to such a change? I have a solution in mind where it would be set by the user, by an environment variable, or would default to a platformdirs location.

This would also apply to the logs directory, which sits in the export hierarchy.

Is there a reason that iam_variables_mapping isn't part of the data directory?

@tngTUDOR
Copy link
Collaborator

tngTUDOR commented Oct 10, 2023

The changes are self-evident in code, but it would be worth it to add them to the CHANGELOG

Copy link
Collaborator

Choose a reason for hiding this comment

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

moves the DATA_DIR related vars to the new filesystem_constants module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed all the internal imports, and these paths were in __all__ so shouldn't be importable in any case?

check_internal_linking,
link_internal,
)
from wurst.linking import change_db_name, check_internal_linking, link_internal
Copy link
Collaborator

Choose a reason for hiding this comment

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

which is the right black formatting ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The CI does black auto-formatting, so we can't make this look any different in any case.

setup.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: let's keep only 1 source of requirements (duplicated in setup.py and requirements.txt)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't forget meta.yaml in conda :) Not sure that one can be anything but duplicative...

@tngTUDOR
Copy link
Collaborator

@romainsacchi @tngTUDOR I would love to get your input on the following:

The current export directory structure could also be refactored, so that the export directory wouldn't depend on the current working directory. Are you open to such a change? I have a solution in mind where it would be set by the user, by an environment variable, or would default to a platformdirs location.

This would also apply to the logs directory, which sits in the export hierarchy.

Is there a reason that iam_variables_mapping isn't part of the data directory?

I agree that having a platformdirs folder, is an elegant way of centralizing files.

@cmutel
Copy link
Collaborator Author

cmutel commented Oct 10, 2023

The CI failing test is outside the changes I made (pandas variable name). @romainsacchi can I mark it skipped?

@romainsacchi romainsacchi merged commit 3933a13 into polca:master Oct 10, 2023
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle cache creation for multi-user envs differently
3 participants