-
Notifications
You must be signed in to change notification settings - Fork 8
update code to use . instead of / #432
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
Conversation
WalkthroughThe changes remove prior string replacement logic in both frontend and backend files. In the frontend, configuration names are now set directly without altering slashes to periods. In the backend, the intermediary Changes
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
⏰ Context from checks skipped due to timeout of 90000ms (17)
🔇 Additional comments (8)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
david-zlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right to me.
the join names or names of any chronon confs should never have /'s -> https://github.com/zipline-ai/chronon/blob/main/api/py/ai/chronon/repo/extract_objects.py#L78
I'll be adding some assertion logic to make sure we join names with /'s don't show up
|
Thanks @ken-zlai! Let me finish this refactor I'm in the middle of then I'll update my local env off this branch. |
sean-zlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ken-zlai looks good, thanks!
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected? ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallback label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary #398 updated the module path from `"/"` to `"."`, but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins. @david-zlai – Can you review the code to ensure it fully aligns with the new convention? @sean-zlai – Can you tear down all Doour clientser images and rebuild on this branch to confirm observability works as expected? ## Cheour clientslist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Streamlined how configuration names are handled in observability views. Names are now displayed as originally provided without extra formatting, ensuring a consistent and straightforward presentation. The fallbaour clients label remains “Unknown” when a name is not available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->

Summary
#398 updated the module path from
"/"to".", but not all code was migrated to the new convention, causing frontend API calls to fail when retrieving joins.@david-zlai – Can you review the code to ensure it fully aligns with the new convention?
@sean-zlai – Can you tear down all Docker images and rebuild on this branch to confirm observability works as expected?
Checklist
Summary by CodeRabbit