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

chore: circular imports and name input #92

Merged
merged 2 commits into from
Dec 6, 2022
Merged

Conversation

fjcasti1
Copy link
Contributor

@fjcasti1 fjcasti1 commented Dec 6, 2022

No description provided.

@fjcasti1 fjcasti1 self-assigned this Dec 6, 2022
@fjcasti1 fjcasti1 marked this pull request as ready for review December 6, 2022 02:22
Comment on lines -73 to +74
return os.path.join(dataset_dir, self.name)
return self.__directory
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

from phoenix.datasets import EmbeddingColumnNames, Schema
from phoenix.datasets.schema import EmbeddingColumnNames, Schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to import this way going forward, or is this a temporary fix? We still have from .schema import EmbeddingColumnNames, Schema in phoenix.datasets.__init__.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is where I personally think it's fine to be importing from ".schema" here - they are part of the same directory / module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to remove from .schema import EmbeddingColumnNames, Schema from phoenix.datasets.__init__.py in that case?

Copy link
Contributor

@mikeldking mikeldking Dec 6, 2022

Choose a reason for hiding this comment

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

I think letting the DX be from phoenix.datasets import Schema, Dataset feels like the best ergonomics.

Does this not work?

classDiagram
    __init__ <|-- Dataset
    Dataset <|-- Schema
     __init__ <|-- Schema
    class Dataset{
      +Schema schema
    
    }
   
Loading

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 will change so that files in the same directory use local imports. Keeping the DX such that they can import useful entities in one line as it is now from phoenix.datasets import Schema, Dataset

@fjcasti1 fjcasti1 merged commit 1775f6c into main Dec 6, 2022
@fjcasti1 fjcasti1 deleted the fix-circular-import branch December 6, 2022 19:32
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.

3 participants