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

util: basic config manager #113

Merged
merged 7 commits into from
Nov 14, 2023
Merged

util: basic config manager #113

merged 7 commits into from
Nov 14, 2023

Conversation

hsirkar
Copy link
Collaborator

@hsirkar hsirkar commented Oct 13, 2023

Usage:

import pipit as pp

pp.set_option("notebook_url", "https://localhost:8080")
pp.reset_option("notebook_url")

pp.get_option("log_level")

pipit/config.py Outdated
# SPDX-License-Identifier: MIT

# Private dict that stores global config values
_config = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good for a basic config manager. Here is a draft implementation in Hatchet: https://github.com/hatchet/hatchet/pull/511/files. It's similar to Pandas set_options. registered_options stores the default settings and global_config stores the current settings. The get_option, set_option, and reset_option functions are straightforward. There are also some validators to check if the given inputs are correct but I think the validator implementation has not finished yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@hsirkar hsirkar requested a review from ocnkr November 10, 2023 16:19
@@ -4,3 +4,4 @@
# SPDX-License-Identifier: MIT

from .trace import Trace # noqa: F401
from .config import get_option, set_option # noqa: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add reset_option

@@ -0,0 +1,47 @@
import pipit as pp

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a function in which we might want to use global config? For example, in Hatchet we have the tree function that prints the CCT. One of the parameters it takes is colormap. We want to change colormap globally using set_option. Do we have a function like this in Pipit? If yes, can we add a test that it works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a set_option function that is exposed to users. I'm not sure if this is what you mean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In Hatchet, if we specify a colormap parameter for the tree function, does it set the colormap globally? Or just once, for that specific tree?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it just changes the colormap for that call. If we want to change it globally, we have to use set_option.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense, we should be able to do the same in Pipit for timeline, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I was asking if we could add a test for something like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I understand what you're getting at, the timeline function would say something like:

def timeline( ..., colormap = None):
    ...
    if colormap = None:
        colormap = pp.get_config("colormap")
    ...

We currently have tests for pp.get_config. I think a specific test for the code above would be in the timeline test? Please correct me if I'm misunderstanding anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think that makes sense. Let's discuss this with @bhatele later.

@bhatele bhatele changed the title Basic config manager util: basic config manager Nov 14, 2023
@bhatele bhatele merged commit b6b5f47 into develop Nov 14, 2023
8 checks passed
@bhatele bhatele deleted the config branch November 14, 2023 23:47
@hsirkar hsirkar mentioned this pull request Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants