File parsing security patches#58
File parsing security patches#58mounacheikho-cmd wants to merge 5 commits intoclimateandtech:maintenance/security-updatesclimateandtech/report-analyst:maintenance/security-updatesfrom mounacheikho-cmd:issue-53-file-parsing-security-patchesmounacheikho-cmd/report-analyst:issue-53-file-parsing-security-patchesCopy head branch name to clipboard
Conversation
| chunk_overlap = st.session_state.new_overlap | ||
| top_k = st.session_state.new_top_k | ||
| llm_model = st.session_state.new_llm_model | ||
| st.session_state.new_llm_scoring = st.session_state.get("llm_scoring_widget", st.session_state.new_llm_scoring) |
There was a problem hiding this comment.
@mounacheikho-cmd Seems like this fixed a bug?
There was a problem hiding this comment.
Yes this fixed a bug I ran into while testing the user flow.
After uploading a file, streamlit could call the update function before the checkbox state existed and that made the widgets and analyzer settings out of sync.
So I fixed it by initializing the session_state values at startup and giving the checkbox widgets their own keys. The update function then copies the widget values back into the analyzer settings so that both sides stay synchronized.
| available_models = get_available_llm_models() | ||
| default_model = available_models[0] if available_models else OPENAI_MODELS[0] | ||
| default_question_set = list(question_sets.keys())[0] if question_sets else "tcfd" | ||
| st.session_state.setdefault("new_chunk_size", 500) |
There was a problem hiding this comment.
Here too, do you remember what was the problem here?
There was a problem hiding this comment.
It was the same kind of issue here. Some upload and cache code can run before the analysis controls appear, so the session state values had not been created yet and that caused the missing new_llm_model error and made the page freeze on the Report Analyst page.
That’s why I initialized the values at the start. I also mentioned this in our last PT.
| selected_index = ( | ||
| available_llm_models.index(current_model) if current_model in available_llm_models else 0 | ||
| ) | ||
| if current_model not in available_llm_models: |
There was a problem hiding this comment.
Interesting. Question: When would session_state.get("new_llm_model") not be in available_llm_models?
Maybe this change is fine as it is, I am not sure about it because of this uncertainity and setting the new model, possibly intransparently to the first available model seems a bit risky to me.
There was a problem hiding this comment.
I didn’t run into this exact case during testing, but I added this check just in case. because streamlit keeps session state between reruns, but the available models can change when API keys or model configuration change. In that situation an old model could remain selected even though it is no longer available.
but you are right I should make this behavior clearer, what do you think of this instead?
current_model = st.session_state.get("new_llm_model")
if current_model not in available_llm_models:
fallback_model = available_llm_models[0]
if current_model:
st.warning(
f"'{current_model}' is no longer available. "
f"The model was reset to '{fallback_model}'."
)
st.session_state.new_llm_model = fallback_model
| help="Batch scoring only applies when LLM scoring is enabled.", | ||
| ) | ||
| if new_llm_scoring: | ||
| new_batch_scoring = st.checkbox( |
There was a problem hiding this comment.
Question: This seems that llm scoring was not working, what was the problem here?
There was a problem hiding this comment.
Yes, I found this while testing. The scoring logic itself wasn’t the problem.
On reruns, the checkbox widgets and the values used by the analyzer could get out of sync, especially before the widgets had initialized.
I gave the widgets separate keys and copied their values back into the analyzer settings so they stay consistent.
|
@suung I cleaned this PR up so it only contains the dependency update. While testing, I ran into the I also removed I reran the tests on the cleaned branch: |
…g-security-patches
Part of #53
This patches the
pypdfsecurity alert and the related file parsing dependency group.The
pypdfupdate could not be done by itself because the old LlamaIndex file reader pin requiredpypdf<6. I updated the related LlamaIndex packages as well.Notes
This ended up being larger than a simple patch bump because
pypdfwas tied to the LlamaIndex file reader stack.During local testing, old LlamaIndex packages from the previous install were still present in my environment and caused dependency conflict messages. I removed those old leftover packages and reinstalled the requirements.
The app also exposed a Streamlit session-state issue on the Report Analyst page. I fixed the widget/default handling so the page loads and navigation works when running the app from the terminal.
Validation
pip check.wheel/packagingandsqlite-vsswarnings.OPENAI_API_KEY=test-key PYTHONPATH="$PWD" python -m pytest219 passed, 4 skipped, 20 warningsManual test checklist
Local run note
Run the app from the terminal:
python -m streamlit run report_analyst/streamlit_app.pyVS Code debug mode may pause on handled Streamlit/dependency warnings, so I used the terminal run for manual testing.
For extra debugging:
python -m streamlit run report_analyst/streamlit_app.py --logger.level=debug --client.showErrorDetails=full