-
Notifications
You must be signed in to change notification settings - Fork 12
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
Update Gen AI for Integration Tests #104
base: main_ea4
Are you sure you want to change the base?
Conversation
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Behrooz <[email protected]>
" },\n", | ||
"}\n", | ||
"\n", | ||
"# for the integration test only\n", |
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.
Hi @drbeh ! Please add this into the test script (test_monai_notebook.py) instead of having it in the notebook. Thanks!
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.
To achieve this, you may need nbformat
to manipulate the cells. For example, this is what I do locally for development mode testing
def remove_download(cells, inplace=True):
# New cell content
cell_copy = cells.copy() if not inplace else cells
# Identify and replace the target cell
def condition(cell):
if cell.cell_type != "code":
return False
return cell.source.startswith("# Download the job artifacts, including logs, scripts, and model checkpoints")
for cell in cell_copy:
if condition(cell):
cell.source = ""
break # Assuming there's only one cell with this marker, break after finding it
nb = nbformat.read(notebook_path, as_version=4)
cells = nb.cells
cells = remove_download(cells)
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.
Hi @mingxin-zheng, I see your point but the way that integration tests are suppose to parametrize these notebooks are through the parameters
cell (the cell tagged with "parameters") and the test cases are defined in NOTEBOOK_TEST_CASES
as a list of dictionaries. Modifying arbitrary cells will break the logic and make it particular to this test.
It makes sense for what you have done which is not notebook dependents but adding specific content to a specific cell of a specific notebooks looks very hacky to me and it is hard to maintain in a long run. What to you think? @mingxin-zheng @Nic-Ma @yiheng-wang-nv
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.
@mingxin-zheng following your suggestion. I added a general rule to overwrite any tagged cell with the requested data. I will test to see if it works.
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.
I tested it and it worked for generative ai notebook.
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Behrooz <[email protected]>
Signed-off-by: Behrooz <[email protected]>
@mingxin-zheng @Nic-Ma I have updated this notebook and removed all the integration test related stuff. Instead, I added |
Testing it right now. Hopefully we can merge this PR soon |
No description provided.