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

Solve issue with manual select not working #55

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Conversation

Joao-Alves
Copy link
Contributor

We were having an issue that the manual select wasn't working when we were using streamlit option menu, even when we copied the example in the readme it wasn't working.

We entered the code of the package to understand how it was working and found that the issue was related with the the if statement that checked if the key was None. To have the manual select you need to have the key set otherwise you can't use it.

In this change I removed it, because there is already a check for the on_change which is essential for the manual select.

I also improved the readme to work better with the manual select. I think the change also answers the issue #46.

I haven't tested, but the error I was getting was similar to issue #27 and from the description I think it will solve it as well.

@victoryhb
Copy link
Owner

Thanks for the PR! Will look into it.
@fgdvir Can you please review this first?

@fgdvir
Copy link
Collaborator

fgdvir commented Nov 27, 2023

@Joao-Alves
The original code does work (and I'm using it in multiple streamlit apps I've built).
Can you please supply an environment yaml and full code that doesn't work for you? (streamlit version and other things you might have installed?, also what version of the option_menu are you using? )

@Joao-Alves
Copy link
Contributor Author

Hello, the issue I have was similar to this one reported in the streamlit forum. From what they are saying is an issue with the streamlit chat input.

https://discuss.streamlit.io/t/st-chat-input-interaction-with-other-menu-option-component/50354

And they have a video showing the issue.

Here are the versions:

  • Streamlit - 1.28.2
  • streamlit-option-menu - 0.3.6

Here is a sample code where you will have the issue:

import streamlit as st
from streamlit_option_menu import option_menu


def main():
  st.markdown("Welcome")

  option_list = ["Q&A", "Summarization", "Compare & Contrast", "Output Parsers", "Admin Panel"]
  manual_select = None

  if st.session_state.get("main_menu", False):
      option_list_option = st.session_state["main_menu"]
      manual_select = option_list.index(option_list_option)

  # menu for document analyzer apps
  summary_type = option_menu(
      None,
      options=option_list,
      icons=[
          "chat-dots-fill",
          "file-earmark-text-fill",
          "zoom-in",
          "table",
          "person-fill-lock",
      ],
      menu_icon="cast",
      manual_select=manual_select,
      orientation="horizontal",
      key="main_menu",
  )
  summary_type

  if summary_type == "Q&A":
      st.markdown("Page 1")
  st.chat_input("Enter your message here")

if __name__ == "__main__":
  main()

https://drive.google.com/file/d/14g8wzRlWvRTzaDl4LTuGCG05O6-15GY1/view?pli=1 - Link for the video

@fgdvir
Copy link
Collaborator

fgdvir commented Nov 30, 2023

@Joao-Alves
Thanks for the detailed reply.
Unfortunately I couldn't reproduce the issue (I installed a new env with your versions and it works as expected, unlike the video you attacked).

If you do have this issue and your fix solves it, I guess I can accept it. the reason for the condition you removed (I think) was because when the key is not None, after the first initialization, the default_index doesn't matter anymore, so it is pointless to change it. Possibly for some reason in some environment (what browser are you using? I'm using arc/chrome

In any case, your readme has a bug in it so please fix so I can merge.

@victoryhb what do you think?

@Joao-Alves
Copy link
Contributor Author

Hello,
I was trying to do a deep dive to see why it wouldn't work on your machine, and I found a bug in my copy and pasted code here to the issue. You need to add a tab on the st.chat_input.

The bug occurs when the chat input widget from streamlit is removed from the rendered page. Using my fix the option-menu works well.

Here is the revised code:

import streamlit as st
from streamlit_option_menu import option_menu


def main():
    st.markdown("Welcome")

    option_list = [
        "Q&A",
        "Summarization",
        "Compare & Contrast",
        "Output Parsers",
        "Admin Panel",
    ]
    manual_select = None

    if st.session_state.get("main_menu", False):
        option_list_option = st.session_state["main_menu"]
        manual_select = option_list.index(option_list_option)

    # menu for document analyzer apps
    summary_type = option_menu(
        None,
        options=option_list,
        icons=[
            "chat-dots-fill",
            "file-earmark-text-fill",
            "zoom-in",
            "table",
            "person-fill-lock",
        ],
        menu_icon="cast",
        manual_select=manual_select,
        orientation="horizontal",
        key="main_menu",
    )
    summary_type

    if summary_type == "Q&A":
        st.markdown("Page 1")
        st.chat_input("Enter your message here")


if __name__ == "__main__":
    main()

To ensure I have a stable I am using Poetry and here is my pyproject.toml:

[tool.poetry]
name = "streamlit-option-menu-bug"
version = "0.1.0"
description = ""
authors = ["João <[email protected]>"]
readme = "README.md"
packages = [{include = "streamlit_option_menu_bug"}]

[tool.poetry.dependencies]
python = ">=3.10,<3.12"
streamlit = "1.28.2"
streamlit-option-menu = "0.3.6"

[build-system]
build-backend = "poetry.core.masonry.api"
requires = ["poetry-core"]

I am also using Edge and Chrome to test this out on Windows. I have also tested with streamlit 1.29.0 and the bug still occurs.

@victoryhb
Copy link
Owner

@fgdvir I trust you to make the appropriate decision on this issue. You can merge it as you deem fit (or let me know if you can't do so directly).

@fgdvir
Copy link
Collaborator

fgdvir commented Dec 12, 2023

@Joao-Alves
Thanks!.
Indeed in the fixed example I got the same issue and your solution seems to fix it . I must say I'm not sure why it behaves this way but since I don't see an issue with this fix I can accept it.

Please fix/revert the README so I can merge your PR

@Joao-Alves
Copy link
Contributor Author

What is the issue you have with the readme?

I reviewed to make it more seamless to use option list and make it more evident the option we are selecting in session state.

@fgdvir
Copy link
Collaborator

fgdvir commented Dec 12, 2023

@Joao-Alves see above issue, I tagged you. the example doesn't work, it yields an error

README.md Show resolved Hide resolved
@Joao-Alves
Copy link
Contributor Author

I reviewed the issue and added a commit. I also think I improved the readme to be more explicit in this case and keep the use case that was there.
@fgdvir what do you think?

If you think the simpler readme is better I am happy to revert both commits.

@fgdvir fgdvir merged commit d3518b2 into victoryhb:master Dec 13, 2023
@Joao-Alves
Copy link
Contributor Author

Thank you for merging.

When do you think it will be available in Pypi the fixed version?

@victoryhb
Copy link
Owner

victoryhb commented Dec 14, 2023

Hi @Joao-Alves @fgdvir, I tested the merged code briefly but the new example code doesn't appear to work. Specifically:

  • Clicking on the fourth menu results in ValueError: False is not in list
  • The "Move to Next" button doesn't work either.

Additional minor problems:

  • The new example code is complex and hard to understand, without a clear purpose.
  • The code isn't properly formatted.

So will have to revert the merge for now.

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.

3 participants