Conversation
ada333
changed the title
README.md
Outdated
| | Cell Type | Status | Description | | ||
| |-----------|--------|-------------| | ||
| | **Imports** | ✅ Works | Use for all import statements. **All imports must be placed in cells tagged as `imports`.** Importing libraries (pandas, tensorflow, etc.) in other cell types will cause pipeline execution errors. | | ||
| | **Functions** | ✅ Works | Use for function and class definitions only. **Do not include** logic, print statements, or import statements in these cells. Only pure definitions should be here. | |
There was a problem hiding this comment.
Function cells can contain logic inside function bodies right? I think what you mean is that function cells should only contain definitions (function/class defs), not top-level
executable statements
README.md
Outdated
| ### Important Guidelines | ||
|
|
||
| > [!WARNING] | ||
| > **Import statements must be in `imports` cells only!** |
There was a problem hiding this comment.
A more accurate statement: imports outside imports cells won't be detected for automatic dependency installation,
which causes ImportError at runtime if the package isn't pre-installed in the container image.
If the package is on the container image works isn't?
There was a problem hiding this comment.
good point, if the package is on the container image it will work. I will add it to Readme.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
README.md
Outdated
| |-----------|--------|-------------| | ||
| | **Imports** | ✅ Works | The code in this cell will be pre-pended to every step of the pipeline. Used for all import statements. **All imports must be placed in cells tagged as `imports`.** Importing libraries (pandas, tensorflow, etc.) in other cell types will cause pipeline execution errors. | | ||
| | **Functions** | ✅ Works |The code in this cell will be pre-pended to every step of the pipeline, after **imports**. Used for function and class definitions only. **Do not include** top-level executable statements | | ||
| | **Pipeline Parameters** | ✅ Works | Define variables that will become pipeline parameters. | |
There was a problem hiding this comment.
Some testing on the Pipeline Parameters label showed that if more than on e Pipeline Parameters cell exists, and a parameter is defined in each cell, only the final value will be taken. For example, parameter cell 1: num_cats = 3 and parameter cell 2: num_cats = 5 will result in num_cats = 5 for the whole pipeline.
Please update this section to say that if the same parameter is defined in mulitple Pipeline Parameter cells, only the value in the last cell will be used.
README.md
Outdated
| ### Important Guidelines | ||
|
|
||
| > [!WARNING] | ||
| > **Imports outside imports cells won't be detected for automatic dependency installation, which causes ImportError at runtime if the package isn't pre-installed in the container image.** |
There was a problem hiding this comment.
Maybe write this as "Imports outside Imports cells..." to be slightly clearer
README.md
Outdated
|
|
||
| **Best Practices:** | ||
| - Place all imports at the beginning of your notebook in cells tagged as `imports` | ||
| - Keep function definitions pure - no side effects, prints, or imports |
There was a problem hiding this comment.
what do you mean by side effects in this context?
There was a problem hiding this comment.
I meant no changing of global variables / parameters.
Signed-off-by: Adam Maly <amaly@redhat.com>
Signed-off-by: Adam Maly <amaly@redhat.com>
Signed-off-by: Adam Maly <amaly@redhat.com>
|
@hmtosi would mind taking a second look here? |
3 participants
This PR adds clear information about cell types in ReadMe mainly for first-time user. I think it is important that we mention somewhere that imports work only in imports cell.