Rename py to python directory because pytest collision with its py directory #543
Conversation
WalkthroughThe changes update the directory naming convention from Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub
participant Workflow
participant Linter
GitHub->>Workflow: Push/Pull Request (api/python/**)
Workflow->>Linter: Execute linting & Thrift generation
Linter-->>Workflow: Return results
Workflow->>GitHub: Upload artifacts with updated paths
Possibly related PRs
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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
changed the title
| **/.vscode/ | ||
| **/__pycache__/ | ||
| **/.DS_Store | ||
| api/py/ai/chronon/ |
There was a problem hiding this comment.
2f50dd4#diff-bc37d034bad564583790a46f19d807abfe519c5671395fd494d8cce506c42947L20-L21
this was incorrectly done
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
docs/source/setup/Data_Integration.md (2)
13-13: Grammar: Consider capitalizing "Python-based" in "python-based CLI" for consistency.🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: The proper noun in this adjective needs to be capitalized.
Context: ...repo/run.py) Python script (this is the python-based CLI entry point for all jobs). We reco...(NNP_BASED)
15-15: Grammar: Replace "setup" with "set up" if intended as a verb.🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: The word “setup” is a noun. The verb is spelled with a white space.
Context: ... docs](./Developer_Setup.md) for how to setup the mainchronondirectory.). If you ...(NOUN_VERB_CONFUSION)
Dockerfile (1)
91-91: Path update confirmed. UpdatedCOPYcommand now usesapi/python/test/samplecorrectly.docs/source/setup/Overview.md (1)
23-23: URL update. The link now correctly points to/api/python/ai/chronon/repo/run.py.docs/source/authoring_features/ChainingFeatures.md (2)
23-24: URL updates confirmed. All links now use the/api/python/prefix.
87-87: Grammar note. Consider adding a comma and “the” for clarity in the sentence introducing downstream computations.🧰 Tools
🪛 LanguageTool
[uncategorized] ~87-~87: You might be missing the article “the” here.
Context: ...ations - The goal of chaining is to use output of a Join as input to downstream comput...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[typographical] ~87-~87: It seems that a comma is missing after this introductory phrase.
Context: ...am computations like GroupBy or a Join. As of today we support the case 1 and case 2 in fut...(AS_OF_COMMA)
docker-init/Dockerfile (2)
24-24: Path update. Updated the copy command to use./api/python/test/sampleinstead of the old directory.
29-29: Path update. Therun.pyfile is now sourced from./api/python/ai/chronon/repo/run.pyas required.docs/source/dev/devnotes.md (1)
14-14: Env var update.CHRONON_APIis now correctly set to$CHRONON_OS/api/python.docs/source/authoring_features/GroupBy.md (2)
115-119: Nitpick: In 'For example if we want…', add a comma after 'For example' and verify preposition use.🧰 Tools
🪛 LanguageTool
[typographical] ~116-~116: After the expression ‘for example’ a comma is usually used.
Context: ... also accept list columns as input. For example if we wantaverageitem_pricefrom ...(COMMA_FOR_EXAMPLE)
[uncategorized] ~118-~118: The preposition ‘of’ seems more likely in this position.
Context: ...r to a column name which contains lists as values. In traditional SQL this would r...(AI_HYDRA_LEO_REPLACE_AS_OF)
[uncategorized] ~119-~119: Possible missing comma found.
Context: ...ontains lists as values. In traditional SQL this would require an expensive `explod...(AI_HYDRA_LEO_MISSING_COMMA)
242-242: Nitpick: Consider inserting a comma in the banner text for clarity.🧰 Tools
🪛 LanguageTool
[typographical] ~242-~242: It appears that a comma is missing.
Context: ...` ## Bucketed GroupBy Example In this example we take the [Purchases GroupBy](https:/...(DURING_THAT_TIME_COMMA)
docs/source/Code_Guidelines.md (2)
58-59: Nitpick: Revise verb form and add period after 'A.K.A' (e.g. 'A.K.A.').🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...d. We have restricted the code base to use implicit only to retroactively extend c...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[uncategorized] ~59-~59: The abbreviation/initialism is missing a period after the last letter.
Context: ...t only to retroactively extend classes. A.K.A as extension objects. Every other use s...(ABBREVIATION_PUNCTUATION)
64-65: Nitpick: A comma might improve clarity after 'JVM'.🧰 Tools
🪛 LanguageTool
[uncategorized] ~65-~65: A comma might be missing here.
Context: ...her language on JVM, in terms of power. Also Spark APIs are mainly in Scala2. ### T...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
.github/workflows/test_python.yaml (1)
81-81: Artifact path updated – new line required.
Ensure a newline is added at the end of the file.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 81-81: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (4)
api/python/test/sample/data/checkouts.csvis excluded by!**/*.csvapi/python/test/sample/data/purchases.csvis excluded by!**/*.csvapi/python/test/sample/data/returns.csvis excluded by!**/*.csvapi/python/test/sample/data/users.csvis excluded by!**/*.csv
📒 Files selected for processing (24)
.github/workflows/test_python.yaml(2 hunks).gitignore(1 hunks)Dockerfile(1 hunks)README.md(12 hunks)api/python/README.md(1 hunks)api/thrift/api.thrift(1 hunks)docker-init/Dockerfile(1 hunks)docs/build-sphinx.sh(1 hunks)docs/source/Code_Guidelines.md(2 hunks)docs/source/Kaggle_Outbrain.md(8 hunks)docs/source/authoring_features/ChainingFeatures.md(2 hunks)docs/source/authoring_features/GroupBy.md(10 hunks)docs/source/authoring_features/Join.md(20 hunks)docs/source/authoring_features/Source.md(2 hunks)docs/source/authoring_features/StagingQuery.md(2 hunks)docs/source/dev/devnotes.md(1 hunks)docs/source/getting_started/Tutorial.md(8 hunks)docs/source/setup/Data_Integration.md(1 hunks)docs/source/setup/Developer_Setup.md(1 hunks)docs/source/setup/Overview.md(1 hunks)scripts/distribution/build_wheel.sh(1 hunks)scripts/distribution/run_aws_quickstart.sh(1 hunks)scripts/distribution/run_gcp_quickstart.sh(1 hunks)scripts/docsite_release/build.sh(3 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/source/setup/Data_Integration.md
[grammar] ~13-~13: The proper noun in this adjective needs to be capitalized.
Context: ...repo/run.py) Python script (this is the python-based CLI entry point for all jobs). We reco...
(NNP_BASED)
[grammar] ~15-~15: The word “setup” is a noun. The verb is spelled with a white space.
Context: ... docs](./Developer_Setup.md) for how to setup the main chronon directory.). If you ...
(NOUN_VERB_CONFUSION)
docs/source/Kaggle_Outbrain.md
[style] ~85-~85: Consider a more expressive alternative.
Context: ...at we want to aggregate clicks by). To do this, we'll write a simple SQL join, an...
(DO_ACHIEVE)
docs/source/authoring_features/GroupBy.md
[typographical] ~116-~116: After the expression ‘for example’ a comma is usually used.
Context: ... also accept list columns as input. For example if we want average item_price from ...
(COMMA_FOR_EXAMPLE)
[uncategorized] ~118-~118: The preposition ‘of’ seems more likely in this position.
Context: ...r to a column name which contains lists as values. In traditional SQL this would r...
(AI_HYDRA_LEO_REPLACE_AS_OF)
[uncategorized] ~119-~119: Possible missing comma found.
Context: ...ontains lists as values. In traditional SQL this would require an expensive `explod...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~242-~242: It appears that a comma is missing.
Context: ...` ## Bucketed GroupBy Example In this example we take the [Purchases GroupBy](https:/...
(DURING_THAT_TIME_COMMA)
docs/source/Code_Guidelines.md
[uncategorized] ~58-~58: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...d. We have restricted the code base to use implicit only to retroactively extend c...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[uncategorized] ~59-~59: The abbreviation/initialism is missing a period after the last letter.
Context: ...t only to retroactively extend classes. A.K.A as extension objects. Every other use s...
(ABBREVIATION_PUNCTUATION)
[uncategorized] ~65-~65: A comma might be missing here.
Context: ...her language on JVM, in terms of power. Also Spark APIs are mainly in Scala2. ### T...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
docs/source/authoring_features/ChainingFeatures.md
[uncategorized] ~87-~87: You might be missing the article “the” here.
Context: ...ations - The goal of chaining is to use output of a Join as input to downstream comput...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[typographical] ~87-~87: It seems that a comma is missing after this introductory phrase.
Context: ...am computations like GroupBy or a Join. As of today we support the case 1 and case 2 in fut...
(AS_OF_COMMA)
docs/source/authoring_features/Join.md
[grammar] ~9-~9: In this case, the usual preposition with “side” is “on”, not “in”. Did you mean “on the left side”?
Context: ...o see how we do this, let's take a look at the left side of the join definition (taken from [Qui...
(IN_ON_THE_RIGHT_HAND_SIDE)
[uncategorized] ~71-~71: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...s between the data a model is trained on and the data used to serve the model. Thes...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~199-~199: Consider a shorter alternative to avoid wordiness.
Context: ...tstrap** as a primitive as part of Join in order to support various kinds of feature experi...
(IN_ORDER_TO_PREMIUM)
[style] ~200-~200: This phrase is redundant. Consider using “outside”.
Context: ...are manually done by clients previously outside of Chronon. Bootstrap is a preprocessing ...
(OUTSIDE_OF)
🪛 YAMLlint (1.35.1)
.github/workflows/test_python.yaml
[error] 81-81: no new line character at the end of file
(new-line-at-end-of-file)
🪛 markdownlint-cli2 (0.17.2)
docs/source/authoring_features/Join.md
452-452: Hard tabs
Column: 1
(MD010, no-hard-tabs)
594-594: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
- GitHub Check: scala_compile_fmt_fix
🔇 Additional comments (42)
api/python/README.md (2)
133-133: Pre-commit Note: Updated instruction with new directory path looks good.
137-137: Git Hook Path: Usingapi/pythonnow is correct.docs/source/setup/Data_Integration.md (1)
17-17: Dir Link: Quickstart example URL now reflectsapi/python; looks correct.docs/source/setup/Developer_Setup.md (1)
37-37: Sample URL: Updated sample link pointing toapi/python/test/sampleis correct.scripts/distribution/build_wheel.sh (2)
3-3: Thrift Gen Update: Output directory changed toapi/pythonas required.
6-6: Wheel Build: The pip wheel command now targetsapi/python; good to go.api/thrift/api.thrift (1)
9-9: Thrift Comment: Updated comment now showsapi/python/in the generation command; looks fine.docs/source/authoring_features/Source.md (2)
21-21: Updated returns link.
All links now referenceapi/python/...correctly.
87-87: Updated users link.
Link now points toapi/python/...as required.README.md (1)
16-319: Consistent path updates.
All links and commands now correctly referenceapi/pythoninstead ofapi/py.🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...ing Chronon offers an API for realtime fetching which returns up-to-date values for you...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~43-~43: Possible missing comma found.
Context: ... range of aggregation types. For a full list see the documentation [here](https://ch...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~183-~183: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...theJoinAPI. For our use case, it's very important that features are computed as of the co...(EN_WEAK_ADJECTIVE)
[uncategorized] ~232-~232: When a number forms part of an adjectival compound, use a hyphen.
Context: ...e computed for that user with a precise 30 day window ending on that timestamp. You c...(MISSING_HYPHEN)
[style] ~246-~246: As an alternative to the over-used intensifier ‘quite’, consider replacing this phrase.
Context: ...olumns, however, note that the table is quite wide and the results might not be very reada...(EN_WEAK_ADJECTIVE)
[uncategorized] ~246-~246: Use a comma before “and” if it connects two independent clauses (unless they are closely connected and short).
Context: ...wever, note that the table is quite wide and the results might not be very readable ...(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~248-~248: Possible missing comma found.
Context: ...adable on your screen. To exit the sql shell you can run: ```shell spark-sql> quit;...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~256-~256: Consider a more expressive alternative.
Context: ...e to productionize the model online. To do this, we need to be able to fetch featu...(DO_ACHIEVE)
[style] ~259-~259: Consider a shorter alternative to avoid wordiness.
Context: ...xt section covers. ### Uploading data In order to serve online flows, we first need the d...(IN_ORDER_TO_PREMIUM)
[style] ~260-~260: Did you mean ‘different from’? ‘Different than’ is often considered colloquial style.
Context: ... the online KV store. This is different than the backfill that we ran in the previou...(DIFFERENT_THAN)
[typographical] ~263-~263: The word “however” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...kups. We use MongoDB in the docker image, however you are free to integrate with a databa...(HOWEVER_SENTENCE)
scripts/distribution/run_aws_quickstart.sh (1)
127-128: CHRONON_ROOT update.
CHRONON_ROOTnow correctly set to$(pwd)/api/python/test/canary.docs/source/Kaggle_Outbrain.md (2)
19-47: Updated PYTHONPATH and navigation.
The export and directory change now useapi/pythoncorrectly.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
28-28: Bare URL used
null(MD034, no-bare-urls)
138-151: Compile/Join commands fixed.
The compile and join command examples properly reference the new directory.docs/build-sphinx.sh (1)
32-33: Sphinx build path update.
Build and install commands now useapi/python, matching the new structure.docs/source/authoring_features/StagingQuery.md (4)
22-23: SQL clause formatting looks clean.
32-33: WHERE clause condition is clear.
59-60: URL update for teams.json is correct.
62-62: Link now points to the updated staging queries; good job.scripts/distribution/run_gcp_quickstart.sh (1)
46-47:CHRONON_ROOTpath updated to api/python; looks solid.docs/source/authoring_features/GroupBy.md (4)
5-5: Intro text updated and clear.
20-21: Key note on selecting the right Source is concise.
23-24: Chaining aggregations note is well-stated.
335-337: Batch Entity example text is clear.docs/source/Code_Guidelines.md (2)
21-32: Scala code samples are concise and readable.
69-72: Testing guidelines are clear and succinct.scripts/docsite_release/build.sh (3)
20-21: Thrift generation commands now target api/python correctly.
35-37: Build and install commands updated to new api/python path.
52-53: Tar command now references the new sample path..github/workflows/test_python.yaml (6)
8-9: Paths updated to "api/python"
The workflow now correctly triggers on the new directory structure.
14-15: Pull-request paths updated
"api/python/**" is used for pull_request events as expected.
43-43: Virtual environment setup looks good.
The venv creation step remains unchanged.
51-51: Changed directory command is correct.
"cd api/python/ai/chronon" reflects the new path.
60-60: Directory change verified.
"cd api/python" is updated correctly in the ruff lint step.
68-72: Thrift commands updated.
All thrift generation commands now output to "api/python/".docs/source/authoring_features/Join.md (4)
9-9: Join URL updated.
The URL now points to the "api/python" directory.🧰 Tools
🪛 LanguageTool
[grammar] ~9-~9: In this case, the usual preposition with “side” is “on”, not “in”. Did you mean “on the left side”?
Context: ...o see how we do this, let's take a look at the left side of the join definition (taken from [Qui...(IN_ON_THE_RIGHT_HAND_SIDE)
12-25: Join code block is unchanged and correct.
The snippet correctly reflects the new directory structure.
89-91: Minor formatting update confirmed.
The "keyMapping" description is clear; no action needed.
403-405: Bootstrap part example is clear.
The inclusion of the production join as a bootstrap part is properly demonstrated..gitignore (1)
22-25: Python paths correctly updated.
The new entries now use the "api/python" prefix.docs/source/getting_started/Tutorial.md (3)
37-38: Raw data URL updated.
The URL now reflects the "api/python" directory.
148-161: Training set Join snippet updated.
The join example now correctly points to "api/python" in the reference link.
290-290: Fetch command remains correct.
The command for fetching join data is unchanged and valid.
|
we'll need to be conscious about backporting fixes from OSS going forward - but not too significant. |
…rectory (#543) Renaming this directory because wasn't able to run intellij debuger/run tests on the ide: https://stackoverflow.com/questions/76625759/cant-run-pytest-with-tmpdir-attributeerror-module-py-has-no-attribute-pat Running the test with `api/py`  versus running with `api/python` (still get an error but different one related to PYTHONPATH and thrift gen)  ## Summary ## 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 ## Summary by CodeRabbit - **Chores** - Standardized naming conventions for Python resources across configuration and build scripts. - **Documentation** - Updated and corrected links and path references in guides, tutorials, and developer materials to reflect the new directory structure. - **Style** - Refined formatting and spacing in documentation for improved clarity and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…rectory (#543) Renaming this directory because wasn't able to run intellij debuger/run tests on the ide: https://stackoverflow.com/questions/76625759/cant-run-pytest-with-tmpdir-attributeerror-module-py-has-no-attribute-pat Running the test with `api/py`  versus running with `api/python` (still get an error but different one related to PYTHONPATH and thrift gen)  ## Summary ## 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 ## Summary by CodeRabbit - **Chores** - Standardized naming conventions for Python resources across configuration and build scripts. - **Documentation** - Updated and corrected links and path references in guides, tutorials, and developer materials to reflect the new directory structure. - **Style** - Refined formatting and spacing in documentation for improved clarity and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…rectory (#543) Renaming this directory because wasn't able to run intellij debuger/run tests on the ide: https://stackoverflow.com/questions/76625759/cant-run-pytest-with-tmpdir-attributeerror-module-py-has-no-attribute-pat Running the test with `api/py`  versus running with `api/python` (still get an error but different one related to PYTHONPATH and thrift gen)  ## Summary ## 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 ## Summary by CodeRabbit - **Chores** - Standardized naming conventions for Python resources across configuration and build scripts. - **Documentation** - Updated and corrected links and path references in guides, tutorials, and developer materials to reflect the new directory structure. - **Style** - Refined formatting and spacing in documentation for improved clarity and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…rectory (#543) Renaming this directory because wasn't able to run intellij debuger/run tests on the ide: https://stackoverflow.com/questions/76625759/cant-run-pytest-with-tmpdir-attributeerror-module-py-has-no-attribute-pat Running the test with `api/py`  versus running with `api/python` (still get an error but different one related to PYTHONPATH and thrift gen)  ## Summary ## 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 ## Summary by CodeRabbit - **Chores** - Standardized naming conventions for Python resources across configuration and build scripts. - **Documentation** - Updated and corrected links and path references in guides, tutorials, and developer materials to reflect the new directory structure. - **Style** - Refined formatting and spacing in documentation for improved clarity and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…rectory (#543) Renaming this directory because wasn't able to run intellij debuger/run tests on the ide: https://staour clientsoverflow.com/questions/76625759/cant-run-pytest-with-tmpdir-attributeerror-module-py-has-no-attribute-pat Running the test with `api/py`  versus running with `api/python` (still get an error but different one related to PYTHONPATH and thrift gen)  ## Summary ## 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 ## Summary by CodeRabbit - **Chores** - Standardized naming conventions for Python resources across configuration and build scripts. - **Documentation** - Updated and corrected links and path references in guides, tutorials, and developer materials to reflect the new directory structure. - **Style** - Refined formatting and spacing in documentation for improved clarity and consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Renaming this directory because wasn't able to run intellij debuger/run tests on the ide:
https://stackoverflow.com/questions/76625759/cant-run-pytest-with-tmpdir-attributeerror-module-py-has-no-attribute-pat
Running the test with
api/pyversus running with
api/python(still get an error but different one related to PYTHONPATH and thrift gen)Summary
Checklist
Summary by CodeRabbit
Summary by CodeRabbit