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

Group all controlling environment variables in single file #2059

Closed
vnlitvinov opened this issue Sep 10, 2020 · 3 comments · Fixed by #2189
Closed

Group all controlling environment variables in single file #2059

vnlitvinov opened this issue Sep 10, 2020 · 3 comments · Fixed by #2189
Assignees
Labels
Code Quality 💯 Improvements or issues to improve quality of codebase new feature/request 💬 Requests and pull requests for new features
Milestone

Comments

@vnlitvinov
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
Currently it's hard both for Modin developer and for end user to see which variables exist and what they do.
I propose to move all variables to a single unified file and replace their usage with getter functions which will have docstrings explaining what variables do.

@vnlitvinov vnlitvinov added new feature/request 💬 Requests and pull requests for new features Code Quality 💯 Improvements or issues to improve quality of codebase labels Sep 10, 2020
@devin-petersohn
Copy link
Collaborator

This will also be important for documentation purposes as well.

@vnlitvinov vnlitvinov self-assigned this Sep 29, 2020
@vnlitvinov vnlitvinov added this to the 0.8.2 milestone Sep 29, 2020
@devin-petersohn
Copy link
Collaborator

I see a couple of options available, not sure which one is better (or maybe we can have both):

1.) Have some kind of global modin.options module where all environment variables set options, and then can be updated with set or retrieved with get. This should make it easy to change to config files as well.
2.) Context-specific configurations via with statement arguments.

A couple of questions that should influence design come to mind:

  • How to ensure local configuration updates are propagated to the whole cluster? It should be easy but we have to make sure it gets done.
  • Can we make certain configurations conditional on the data or workload parameters?

@vnlitvinov
Copy link
Collaborator Author

After some discussion with Devin my current implementation idea is the following:

  1. Move Publisher from modin/__init__.py and generalize it enough so it can retrieve values from os.environ by itself
  2. Make all env variables used now utilize this Publisher
  3. Create a follow-up thread/ticket to discuss and design the (eventual) Modin configuration system, as for now it's almost non-existant (being managed by randomly placed environment variables), and gathering them in one place is just the first step - we'd like to be able to update the variable and have it propagated to all workers, have some dependencies (like make Modin pick Ray as engine if OmniSci is picked as backend), etc.

vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
vnlitvinov added a commit to vnlitvinov/modin that referenced this issue Oct 12, 2020
devin-petersohn pushed a commit that referenced this issue Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Quality 💯 Improvements or issues to improve quality of codebase new feature/request 💬 Requests and pull requests for new features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants