Skip to content

Added new namespaces for icechunk #1189

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: icechunk2
Choose a base branch
from
Open

Added new namespaces for icechunk #1189

wants to merge 5 commits into from

Conversation

@Anu-Ra-g
Copy link

This PR is part of for the fix #1172. I've added new namespaces in this PR as per this doc: #1179

TomNicholas
TomNicholas reviewed Aug 21, 2025
View reviewed changes
Copy link
Contributor

@TomNicholas TomNicholas left a comment

Choose a reason for hiding this comment

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

Thanks for starting work on this!

We should alter the top-level imports in __init__.py to import from these new namespaces.

.github/workflows/rust-ci.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

None of the changes to this file should be in this PR - somethings gone wrong with your usage of git here 😕

Copy link
Author

Choose a reason for hiding this comment

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

I mentioned this issue here. I made the PR to the icechunk2 branch and not the main branch like @paraseba asked.

Copy link
Author

@Anu-Ra-g Anu-Ra-g Aug 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I tried to raise the PR again with a fresh new fork, but the issue was still there.

Copy link
Author

Choose a reason for hiding this comment

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

@TomNicholas Should I raise PR on the main branch then?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, definitely this should be against the icechunk2 branch. The question is how @paraseba's commit ended up there too.

Copy link
Author

Choose a reason for hiding this comment

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

image

I tried to raise a rough PR but still the same issue. This is new for me on Github.

icechunk-python/python/icechunk/config.py Outdated
RepositoryConfig,
initialize_logs,
set_logs_filter
) No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also add the names to python's __all__ variable in each file.

@Anu-Ra-g
Copy link
Author

The usage of pyi file was a little new for me. The types mentioned there are for the Rust code?

@TomNicholas TomNicholas added python 🐍 Related to python code API design 🎨 python Pull requests that update Python code labels Aug 21, 2025
@TomNicholas
Copy link
Contributor

The types mentioned there are for the Rust code?

Yes I believe they are auto-generated from the rust code.

@paraseba
Copy link
Collaborator

I bet @Anu-Ra-g started with a checkout of main instead of icechunk2. I just merged main into the branch. You should try rebasing your fork @Anu-Ra-g.

Re: .pyi file, unfortunately, we have to maintain this file manually, which is a pain

@Anu-Ra-g
Copy link
Author

I bet @Anu-Ra-g started with a checkout of main instead of icechunk2. I just merged main into the branch. You should try rebasing your fork @Anu-Ra-g.

Re: .pyi file, unfortunately, we have to maintain this file manually, which is a pain

Yes, I actually branched out from main and then raised a PR against icechunk2 😅 . I fixed it.

@Anu-Ra-g
Copy link
Author

@TomNicholas I've the __all__ variable for existing namespaces. Can you please check?

TomNicholas
TomNicholas reviewed Aug 22, 2025
View reviewed changes
icechunk-python/python/icechunk/credentials.py Outdated
return res


__all__ = [
Copy link
Contributor

Choose a reason for hiding this comment

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

__all__ conventionally should go at the top of the file, just underneath the imports.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@Anu-Ra-g
Copy link
Author

@TomNicholas Can you please review and suggest the updates?

@Anu-Ra-g
Copy link
Author

Anu-Ra-g commented Sep 2, 2025

@TomNicholas Can you help me out with this CI check? Its failing and I can't tell why?

@paraseba
Copy link
Collaborator

paraseba commented Sep 2, 2025

Don't worry about the Rust failures @Anu-Ra-g . Those are expected because you don't have permission to run with secrets. But the python lint issues are real

@Anu-Ra-g
Copy link
Author

Anu-Ra-g commented Sep 2, 2025 •
edited
Loading

@paraseba python checks are part of the pre-commit config?

@paraseba
Copy link
Collaborator

paraseba commented Sep 2, 2025

@paraseba python checks are part of the pre-commit config?

If you install just, you can run just pre-commit-python locally.

@Anu-Ra-g
Copy link
Author

Anu-Ra-g commented Sep 2, 2025

pre-commit-config is at the root level. just should be installed as part of the virtual environment or by default as part of my system? Why just is not part of the pyproject.toml file? I actually confused as contributing guide doesn't specify these !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomNicholas TomNicholas TomNicholas left review comments

Assignees

No one assigned

Labels

API design 🎨 python 🐍 Related to python code python Pull requests that update Python code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Anu-Ra-g @TomNicholas @paraseba