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

Simplify markdown table generation in magic command - %ai list #1251

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

keerthi-swarna
Copy link
Collaborator

@keerthi-swarna keerthi-swarna commented Feb 19, 2025

Issue Reference: Issue-118

Description

Refactored the code responsible for generating markdown tables by replacing custom imperative logic with the py-markdown-table library.

Changes

  • Updated the _ai_list_command_markdown method in magics.py to generate markdown tables using the py-markdown-table library.
  • Modified the _ai_env_status_for_provider_markdown method to return the env_variable alongside its status in emoji form, replacing the previous approach where the emoji was appended to the env_variable string.
  • Added the py-markdown-table library as a dependency in pyproject.toml.
  • Updated the help text in the aws.py file to address an issue where markdown was not rendering correctly within a markdown table cell. To resolve this, markdown syntax was replaced with HTML tags to ensure proper rendering.

Testing

  • Verified the changes by running Jupyter Lab in a local environment setup.
  • Attached screenshots from the local environment for reference.
Screenshot 2025-02-19 at 6 43 35 PM Screenshot 2025-02-19 at 6 43 44 PM Screenshot 2025-02-19 at 6 43 55 PM

@keerthi-swarna keerthi-swarna added good first issue Good for newcomers enhancement New feature or request labels Feb 19, 2025
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@keerthi-swarna Great work! I left some feedback for you below.

One other issue I noticed is that the py-markdown-table dependency is not available on Conda Forge. Conda Forge is an alternative package registry that allows users to install jupyter-ai using conda, mamba, or micromamba. We can't merge this PR until that is available on Conda Forge, as otherwise installing Jupyter AI from Conda Forge would be broken.

Creating a new Conda Forge package from an existing PyPI package will be a challenge. I would love for you to own this because it is a great opportunity to develop a unique & useful skill. However, I also understand if you need more time before tackling this. Let's talk about this in our next 1:1.

@dlqqq dlqqq marked this pull request as draft February 20, 2025 15:50
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

As you called out, we need to simplify the Markdown output of %ai list considerably in order for it to render well within ipython.

One reason the Markdown table is so complicated is because it is poorly structured. Each row (listing a single provider) actually contains several rows of model IDs in the "model IDs" column. The rows of this table really should just be model IDs, not providers.

Here's how I recommend that we move forward on this:

  1. Create 1 Markdown table, where each row is a unique model ID.

  2. This table should have 3 columns in this order: "Provider name, model ID, environment variable."

  3. The emoji that indicates whether it is set (✅/❌) should be in the "Environment variable" column, i.e. we should merge these 2 columns into 1 to save horizontal space.

@keerthi-swarna
Copy link
Collaborator Author

From above conversation

sharing the images based on the recommendation. Just wanted to confirm if this meets the expectations.
If it does, the alignment in the IPython output is still off due to the help text being too large. We’ll need to either replace the help text with a default message, regardless of the model, or consider using a different approach to generate the text output.

Notebook Output

Screenshot 2025-03-03 at 2 41 46 PM Screenshot 2025-03-03 at 2 42 04 PM

Ipython Output

Screenshot 2025-03-03 at 2 54 07 PM Screenshot 2025-03-03 at 2 54 24 PM

Ipython Output with Default Text

Screenshot 2025-03-03 at 2 58 04 PM Screenshot 2025-03-03 at 2 58 25 PM

Comment on lines +389 to +395
# Generate markdown table for aliases
alias_markdown_table_header = "\n\n Aliases and custom commands:\n"
alias_markdown_table = (
markdown_table(alias_list)
.set_params(quote=False, row_sep="markdown")
.get_markdown()
)
Copy link
Member

Choose a reason for hiding this comment

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

@keerthi-swarna Thanks for diving deep and experimenting. In the _ai_list_command_markdown() method, we use row_sep="markdown" to strictly follow the Markdown spec and allow JupyterLab to render our table in the browser. However, this fails for tables with large columns (like bedrock-custom) when displayed in IPython, since we show the literal string value of the table there.

To render a table row with a very long string in one column in IPython, you could explore using py_markdown_table and set row_sep="always" in _ai_list_command_text(). This would allow for table rows to occupy multiple lines, which would solve the issue you're reporting in IPython. The documentation page includes examples of how setting row_sep="always" can produce readable large tables in IPython. Can you try that and let me know how it goes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it. But still it has some issues with help text being displayed. Let us have a working session offline and close this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify Markdown table generation in magic commands
3 participants