Skip to content
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

keywords for working with multiple open notebook tabs #47

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bdattoma
Copy link

@bdattoma bdattoma commented Dec 22, 2021

Signed-off by Berto D'Attoma: [email protected]

Hi, the following PR wants to propose some keywords to work where there are more than one notebook file opened in JupyterLab. Without these, there were issues trying to get cell outputs with the already present keywords.

implemented keywords

The keywords are:

  1. Wait Until a Given JupyterLab Code Cell Is Not Active In a Given Tab - it waits until the [*] symbol disappear (so the cell finished to run) - renamed to Wait Until JupyterLab Code Cell Is Idle. It works fine in Jupyter v3 and v2.
  2. Get A JupyterLab Code Cell Output In a Given Tab - it returns the output of the cell.
  3. Select Tab By Name - it selects an open tab (notebook file). Renamed to Select JupyterLab Dock Panel Tab By FilePath. It works fine in Jupyter v3, v2 and v1
  4. Get Selected Tab ID - it returns the tab id of the selected one (e.g., tab-key-0). Renamed to Get Selected JupyterLab Dock Panel Tab. It works fine in Jupyter v3 and v2

compatibility with Jupyter v1

The main issue is that in v1 we don't have the id attribute on the "tab" element. I can investigate a way to achieve the same result of keyword 1 and 2 without using the tab id (e.g., by targeting the tab by filepath, like done in keyword 3)

Get Selected Tab ID
[Documentation] Return the tab ID of the selected notebook tab
${active-nb-tab} = Get WebElement xpath:${JL_TABBAR_SELECTED_XPATH}
${tab-id} = Get Element Attribute ${active-nb-tab} id
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like a useful capability.

For consistency, it should be added in Shell.robot (well, now Shell.resource) and probably something more like the existing keywords, e.g. Get JupyterLab Dock Panel Tab so perhaps Get Selected JupyterLab Dock Panel Tab and likely return the actual web element.

This can be a one-shot, as Get Element Attribute will accept xpath:. Further, it should

I don't see where JL_TABBAR_SELECTED_XPATH is defined, but would expect to conform to the (increasingly ridiculous, sorry) naming convention.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, yeah sorry the JL_TABBAR_SELECTED_XPATH is missing, my bad. I will fix it.

Thanks for the review. I will go through all the comments asap.

Copy link
Author

@bdattoma bdattoma Jan 3, 2022

Choose a reason for hiding this comment

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

@bollwyvl I move Get Selected JupyterLab Dock Panel Tab in Shell.resource and instead of returning only the tab ID, I return both the web element and the id. In this way i think the keyword is more useful to the user.
Do you agree with this solution?

I also made the xpath compliant with the naming convention you adopted.

${tab-id} = Get Element Attribute ${active-nb-tab} id
[Return] ${tab-id}

Select Tab By Name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar, as above re: naming convention, and I don't see where JL_TABBAR_CONTENT_XPATH is defined.

As other tab-related things, what happens if there's more than one?

Copy link
Author

Choose a reason for hiding this comment

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

As other tab-related things, what happens if there's more than one?

I changed the keyword to select the tab by filepath instead of filename. So, if there are tabs with same name but different filepath, it still works. What do you think?

@@ -39,3 +39,16 @@ Screenshot Each Output of Active JupyterLab Document
Screenshot Each Output of Active JupyterLab Cell ${prefix}_cell_${i}
Screenshot Markdown of Active JupyterLab Cell ${prefix}_cell_${i}
END

Wait Until a Given JupyterLab Code Cell Is Not Active In a Given Tab
[Documentation] Waits until the given cell in a specific JL tab no longer has an active prompt "[*]:".
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is... very verbose. Could this be broken down into multiple concepts? e.g.

  • get a handle to the ${cell}
    • it might have just been added, or be selected by other means
  • (maybe) go off and do other stuff
  • Wait Until JupyterLab Cell is Idle ${cell}

Also the terminology (idle, active, whatever) probably needs some investigation in either the DOM or message protocol.

Copy link
Author

Choose a reason for hiding this comment

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

not sure I got what you mean by splitting into multiple concepts. i think it is useful to have a such high level keyword to use while running code cell.

I've just changed the keyword name to: Wait Until JupyterLab Code Cell Is Idle.

[Documentation] Waits until the given cell in a specific JL tab no longer has an active prompt "[*]:".
... The cell index starts from 1 and you must count empty and markdown cells too.
[Arguments] ${tab_id_to_wait} ${cell_n} ${timeout}=120seconds
Wait Until Element Is Not Visible xpath=//div[@aria-labelledby="${tab_id_to_wait}"]/${NB TAB XP CONTENT}/div[${cell_n}]/${CELL XP INPUT STATUS ICON} ${timeout}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Many of these aria-label attributes were only added very recently, and won't work with the older versions of lab under test. This is maybe fine, but it's worth exploring some alternate approaches to finding them.

Also, the 2 minutes is ridiculously long as a default. I guess it's all relative anyway but otherwise the longest one is 30s, and I'd recommend adopting that one as a default.

Out of scope of this: we should see whether SeleniumLibrary already has some related ones, and if so, use them (perhaps with some ghetto math). Otherwise, we could name and categorize our own defaults, e.g. ${RFJL DEFAULT WAIT LONG}, ${RFJL DEFAULT WAIT SHORT} that could be easily overriden at the suite level (or updated from the CLI when retrying).

Copy link
Author

Choose a reason for hiding this comment

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

retro-compatibility

I tried this keyword with JL 3.2.5, 2.3.2 and 1.2.21. I see that it works for both v3 and v2. It's not working for v1. Will update the PR description to include the compatibilities.

timeout

agree with you. I've updated it to 30 seconds.

Out of scope of this: we should see whether SeleniumLibrary already has some related ones [...]

agree. We could investigate this

Get A JupyterLab Code Cell Output In a Given Tab
[Documentation] It returns the output of a given cell in a specific JL tab.
... The cell index starts from 1 and you must count empty and markdown cells too.
[Arguments] ${tab_id_to_read} ${cell_n}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one's tough. Also there can be many outputs, which many be worth parameterizing. Anyhow, I'd probably:

  • get a handle on the cell
  • get a handle on the output
  • then use SL's Get Text on the element

Copy link
Author

Choose a reason for hiding this comment

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

I didn't consider other output type (e.g., images) other than text. So, to write a more generic keyword, I thought to return all the output child elements. I could include a parameter to fetch one specific cell by its progressive number. After that, I could write a keyword to extract the text or the image (i'm not sure if there are more than two output types for jupyter)

${CELL XP INPUT STATUS ICON} ${CELL XP INPUT}/div[contains(@class,"jp-InputArea-prompt") and (.="[*]:")]
${CELL XP OUTPUT} div[contains(@class, jp-Cell-outputWrapper)]/div[contains(@class,"jp-Cell-outputArea")]
${CELL XP OUTPUT TEXT} ${CELL XP OUTPUT}//div[contains(@class,"jp-RenderedText")]
${NB TAB XP CONTENT} div[@aria-label="notebook content"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will almost certainly not work with other lab versions.

${JLAB XP DOCK TAB LABEL} ${JLAB XP DOCK TAB}/div[contains(@class, 'p-TabBar-tabLabel')]
${JLAB XP DOCK TAB SELECTED} ${JLAB XP DOCK TABBAR CONTENT}/li[contains(@class,"lm-mod-current p-mod-current")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

please just use p-mod-current (the lm-mod-current ones won't work with lab 1)

[Documentation] Waits until the given cell in a specific JL tab no longer has an active prompt "[*]:".
... The cell index starts from 1 and you must count empty and markdown cells too.
[Arguments] ${tab_id_to_wait} ${cell_n} ${timeout}=30
Wait Until Element Is Not Visible xpath=//div[@aria-labelledby="${tab_id_to_wait}"]/${JLAB XP NOTEBOOK CONTENT}/div[${cell_n}]/${JLAB XP CELL INPUT STATUS ICON} ${timeout}
Copy link
Collaborator

Choose a reason for hiding this comment

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

for consistency, first argument should problaby start with xpath: rather than xpath=

Wait Until JupyterLab Code Cell Is Idle
[Documentation] Waits until the given cell in a specific JL tab no longer has an active prompt "[*]:".
... The cell index starts from 1 and you must count empty and markdown cells too.
[Arguments] ${tab_id_to_wait} ${cell_n} ${timeout}=30
Copy link
Collaborator

Choose a reason for hiding this comment

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

please include the unit, e.g 30s

Copy link
Collaborator

Choose a reason for hiding this comment

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

some default of cell_n (e.g. the last) might be helpful.

@bollwyvl
Copy link
Collaborator

The force pushing makes it hard to track comments over time.

Generally, these are looking okay, but need representative acceptance tests added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants