Feat: Added Model sharing through links #2727
Conversation
|
Performance benchmarks:
|
There was a problem hiding this comment.
Is this the most logical place for this file? (also looking at my co-maintainers)
|
@Sahil-Chhoker This is very cool! My initial thoughts, to which I am not tied) would be
Regardless, this is a great idea and feature! |
|
Thanks for your reply @tpike3
Makes sense, will do!
It makes sense to not make it a default button, but some things I want to clear are that the viz. won't break even if the button does not work as expected, in such case on clicking on the button an empty solara pycafe project opens.
Makes sense, will do! |
WalkthroughThe changes introduce a new module in Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant V as SolaraViz Component
participant M as model_share Module
U->>V: Click "Open on PyCafe" button
V->>M: Call get_pycafe_link(files, requirements, autodetect)
M-->>V: Return PyCafe URL
V->>U: Open URL in new browser tab
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedReview triggered.
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
mesa/visualization/solara_viz.py (1)
134-141: Button functionality looks good, consider renaming for clarityThe implementation of the "Open on PyCafe" button is properly integrated into the AppBar and correctly uses the
get_pycafe_linkfunction withautodetect_files=True. The button styling and target="_blank" attribute ensure a good user experience.Based on the PR comments, consider renaming this to "Share with PYCAFE" as suggested by reviewer tpike3 to make its purpose clearer for users unfamiliar with PyCafe.
solara.Button( - label="Open on PyCafe", + label="Share with PYCAFE", color="blue", attributes={ "href": get_pycafe_link(autodetect_files=True), "target": "_blank", }, )mesa/model_share.py (2)
77-87: Consider enhancing _scan_python_files functionThe
_scan_python_filesfunction is currently limited to specific filenames in the current directory. Consider enhancing it to:
- Support recursive directory scanning (Python modules can be in subdirectories)
- Allow customizing the list of file patterns to scan
-def _scan_python_files(directory_path: str = ".") -> list[str]: - """Scan a directory for specific Python files (model.py, app.py, agents.py).""" - path = Path(directory_path) - python_files = [ - str(file) - for file in path.glob("*.py") - if file.name in ["model.py", "app.py", "agents.py"] - ] - - return python_files +def _scan_python_files(directory_path: str = ".", + file_patterns: list[str] = None, + recursive: bool = False) -> list[str]: + """Scan a directory for specific Python files. + + Args: + directory_path: Path to the directory to scan + file_patterns: List of filename patterns to include (defaults to model.py, app.py, agents.py) + recursive: Whether to scan subdirectories recursively + + Returns: + List of Python file paths matching the patterns + """ + path = Path(directory_path) + if file_patterns is None: + file_patterns = ["model.py", "app.py", "agents.py"] + + pattern = "**/*.py" if recursive else "*.py" + python_files = [ + str(file) + for file in path.glob(pattern) + if file.name in file_patterns + ] + + return python_files
1-87: Add unit tests for the new model_share moduleThe static analysis shows that many lines in this file aren't covered by tests. It's important to add comprehensive unit tests for this functionality to ensure it works correctly and prevent regressions in the future.
Test cases should cover:
- Basic functionality with manually specified files
- Auto-detection of files
- Error handling (missing files, invalid parameters)
- URL generation with different types of input
Consider using mocking to test file operations without depending on actual files.
Would you like me to help create a basic test file structure for this module?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 47-47: mesa/model_share.py#L47
Added line #L47 was not covered by tests
[warning] 49-49: mesa/model_share.py#L49
Added line #L49 was not covered by tests
[warning] 51-54: mesa/model_share.py#L51-L54
Added lines #L51 - L54 were not covered by tests
[warning] 57-58: mesa/model_share.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 60-60: mesa/model_share.py#L60
Added line #L60 was not covered by tests
[warning] 62-63: mesa/model_share.py#L62-L63
Added lines #L62 - L63 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mesa/model_share.py(1 hunks)mesa/visualization/solara_viz.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
mesa/visualization/solara_viz.py (1)
mesa/model_share.py (1)
get_pycafe_link(11-74)
🪛 GitHub Check: codecov/patch
mesa/model_share.py
[warning] 47-47: mesa/model_share.py#L47
Added line #L47 was not covered by tests
[warning] 49-49: mesa/model_share.py#L49
Added line #L49 was not covered by tests
[warning] 51-54: mesa/model_share.py#L51-L54
Added lines #L51 - L54 were not covered by tests
[warning] 57-58: mesa/model_share.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 60-60: mesa/model_share.py#L60
Added line #L60 was not covered by tests
[warning] 62-63: mesa/model_share.py#L62-L63
Added lines #L62 - L63 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows, 3.13)
🔇 Additional comments (2)
mesa/model_share.py (2)
11-36: Function signature and documentation look goodThe function signature with optional parameters and clear type hints is well designed. The docstrings follow a good format with proper description, arguments explanation, examples, and return value description.
65-74: JSON encoding looks well implementedThe JSON encoding, compression, and URL creation logic is appropriately implemented using standard libraries. The compression with gzip and base64 encoding is a good practice to shorten the URL while maintaining the data integrity.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
mesa/model_share.py (1)
1-87: 💡 Verification agent🧩 Analysis chain
Add unit tests to improve code coverage.
The static analysis shows several uncovered lines in this module. Adding unit tests would improve reliability and prevent regressions.
🏁 Script executed:
#!/bin/bash # Check for existing test files that might be related to model_share.py echo "Checking for existing test files for model_share.py:" fd -t f "test_.*model_share.*\.py" || echo "No existing test files found" echo -e "\nChecking Mesa test structure:" fd -t f "test_.*\.py" . -d 1 | head -n 5Length of output: 269
Action Required: Add Unit Tests for
mesa/model_share.pyThe static analysis confirms that there are no existing unit tests covering this module. To improve code coverage and prevent regressions, please add tests that verify:
- The behavior of
get_pycafe_linkin both scenarios:
- When file paths are provided via the
filesparameter.- When using autodetection with
autodetect_files=True.- The functionality of
_scan_python_files, ensuring it correctly filters for"model.py","app.py", and"agents.py".Adding these tests will help ensure that key code paths are exercised and improve overall reliability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 47-47: mesa/model_share.py#L47
Added line #L47 was not covered by tests
[warning] 49-49: mesa/model_share.py#L49
Added line #L49 was not covered by tests
[warning] 51-54: mesa/model_share.py#L51-L54
Added lines #L51 - L54 were not covered by tests
[warning] 57-58: mesa/model_share.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 60-60: mesa/model_share.py#L60
Added line #L60 was not covered by tests
[warning] 62-63: mesa/model_share.py#L62-L63
Added lines #L62 - L63 were not covered by tests
♻️ Duplicate comments (2)
mesa/model_share.py (2)
37-43: 🛠️ Refactor suggestionAdd parameter validation to prevent conflicting options.
The function currently doesn't validate that
filesandautodetect_filesaren't both specified, which could lead to unexpected behavior.requirements = ( requirements or "mesa\nmatplotlib\nnumpy\nnetworkx\nsolara\naltair\npandas" ) +# Validate parameters +if autodetect_files and files: + raise ValueError("Cannot specify both 'files' and 'autodetect_files=True'") + +if not autodetect_files and not files: + raise ValueError("Must provide either 'files' or set 'autodetect_files=True'") + app = "" file_list = []
44-64: 🛠️ Refactor suggestionAdd error handling for file operations.
The current implementation doesn't handle potential file access errors, which could cause the function to fail unexpectedly.
if autodetect_files: all_files = _scan_python_files() for file in all_files: - with open(file) as f: - if file.endswith("app.py"): - app += f.read() - else: - file_dict = {} - file_dict["name"] = os.path.basename(file) - file_dict["content"] = f.read() - file_list.append(file_dict) + try: + with open(file) as f: + if file.endswith("app.py"): + app += f.read() + else: + file_dict = {} + file_dict["name"] = os.path.basename(file) + file_dict["content"] = f.read() + file_list.append(file_dict) + except (IOError, OSError) as e: + print(f"Warning: Could not read file {file}: {e}") else: for file in files: - with open(files[file]) as f: - file_content = f.read() - if file == "app.py": - app += file_content - else: - file_dict = {"name": file, "content": file_content} - file_list.append(file_dict) + try: + with open(files[file]) as f: + file_content = f.read() + if file == "app.py": + app += file_content + else: + file_dict = {"name": file, "content": file_content} + file_list.append(file_dict) + except (IOError, OSError) as e: + print(f"Warning: Could not read file {files[file]}: {e}")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 47-47: mesa/model_share.py#L47
Added line #L47 was not covered by tests
[warning] 49-49: mesa/model_share.py#L49
Added line #L49 was not covered by tests
[warning] 51-54: mesa/model_share.py#L51-L54
Added lines #L51 - L54 were not covered by tests
[warning] 57-58: mesa/model_share.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 60-60: mesa/model_share.py#L60
Added line #L60 was not covered by tests
[warning] 62-63: mesa/model_share.py#L62-L63
Added lines #L62 - L63 were not covered by tests
🧹 Nitpick comments (3)
mesa/visualization/solara_viz.py (1)
134-141: Consider renaming the button to "Share with PYCAFE" for clarity.The button functionality is well-implemented, but as mentioned in the PR feedback from user tpike3, the label "Open on PyCafe" might not be clear enough for users unfamiliar with PyCafe. Changing it to "Share with PYCAFE" would better communicate the button's purpose.
solara.Button( - label="Open on PyCafe", + label="Share with PYCAFE", color="blue", attributes={ "href": get_pycafe_link(autodetect_files=True), "target": "_blank", }, )mesa/model_share.py (2)
65-74: Consider adding compression level control for URLs with large codebases.For projects with substantial code, the URL might become very long. Adding compression level control could help optimize the URL length.
json_object = {"code": app, "requirements": requirements, "files": file_list} json_text = json.dumps(json_object) # Compress using gzip to make the url shorter -compressed_json_text = gzip.compress(json_text.encode("utf8")) +# Use high compression level (9 is maximum) for potentially large codebases +compressed_json_text = gzip.compress(json_text.encode("utf8"), compresslevel=9) # Encode in base64 base64_text = base64.b64encode(compressed_json_text).decode("utf8") c = quote(base64_text) url = f"https://py.cafe/snippet/solara/v1#c={c}"
77-87: Consider expanding file detection to include more Python files.The current implementation only looks for three specific filenames, which might be too restrictive for some projects with different naming conventions.
def _scan_python_files(directory_path: str = ".") -> list[str]: - """Scan a directory for specific Python files (model.py, app.py, agents.py).""" + """Scan a directory for Python files to include in the PyCafe share. + + Looks for key model files first (model.py, app.py, agents.py), then includes + other Python files that might be dependencies. + """ path = Path(directory_path) - python_files = [ + # Primary files that are expected for Mesa models + primary_files = [ str(file) for file in path.glob("*.py") if file.name in ["model.py", "app.py", "agents.py"] ] + + # If no primary files found, include all Python files in the directory + if not primary_files: + return [str(file) for file in path.glob("*.py")] + + return primary_files
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mesa/model_share.py(1 hunks)mesa/visualization/solara_viz.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
mesa/visualization/solara_viz.py (1)
mesa/model_share.py (1)
get_pycafe_link(11-74)
🪛 GitHub Check: codecov/patch
mesa/model_share.py
[warning] 47-47: mesa/model_share.py#L47
Added line #L47 was not covered by tests
[warning] 49-49: mesa/model_share.py#L49
Added line #L49 was not covered by tests
[warning] 51-54: mesa/model_share.py#L51-L54
Added lines #L51 - L54 were not covered by tests
[warning] 57-58: mesa/model_share.py#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 60-60: mesa/model_share.py#L60
Added line #L60 was not covered by tests
[warning] 62-63: mesa/model_share.py#L62-L63
Added lines #L62 - L63 were not covered by tests
🔇 Additional comments (2)
mesa/visualization/solara_viz.py (1)
40-40: LGTM! Appropriate import statement added.The import statement correctly references the new
get_pycafe_linkfunction from the model_share module.mesa/model_share.py (1)
11-36: Function signature and documentation look good.The function signature is clear with well-documented parameters and return type. The docstring is comprehensive and includes a helpful example.
| label="Open on PyCafe", | ||
| color="blue", | ||
| attributes={ | ||
| "href": get_pycafe_link(autodetect_files=True), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for link generation.
The current implementation assumes get_pycafe_link will always succeed, but file operations might fail if the required files aren't found or are inaccessible. Consider implementing error handling to provide a graceful fallback.
solara.Button(
label="Open on PyCafe",
color="blue",
attributes={
- "href": get_pycafe_link(autodetect_files=True),
+ "href": _get_pycafe_link_with_fallback(),
"target": "_blank",
},
)
+# Add this helper function elsewhere in the file
+def _get_pycafe_link_with_fallback():
+ try:
+ return get_pycafe_link(autodetect_files=True)
+ except Exception as e:
+ _mesa_logger.warning(f"Failed to generate PyCafe link: {e}")
+ return "https://py.cafe/snippet/solara/v1" # Fallback to empty project📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "href": get_pycafe_link(autodetect_files=True), | |
| solara.Button( | |
| label="Open on PyCafe", | |
| color="blue", | |
| attributes={ | |
| - "href": get_pycafe_link(autodetect_files=True), | |
| + "href": _get_pycafe_link_with_fallback(), | |
| "target": "_blank", | |
| }, | |
| ) | |
| # Add this helper function elsewhere in the file | |
| def _get_pycafe_link_with_fallback(): | |
| try: | |
| return get_pycafe_link(autodetect_files=True) | |
| except Exception as e: | |
| _mesa_logger.warning(f"Failed to generate PyCafe link: {e}") | |
| return "https://py.cafe/snippet/solara/v1" # Fallback to empty project |
Summary
This PR provides a basic model sharing through a button on the solara web-interface. It uses PyCafe to generate links.
Motive
As explained in #2724. Closes #2724.
Implementation
A new module named
model_share.pyis added to mesa that handles the link generating and a new button labeled "Open on PyCafe" on the solara'sAppBar, that redirects to the generated link that contains all the files your model uses. Strict naming convention is followed for autodetection i.e.model.py,agents.pyandapp.py.Additional Notes
There are many uncertainties and assumptions in this implementation like the button only works if you are present in that directory and I am unsure about if this is even the correct approach to handle this. But having a draft to work with will make things easier that's why this PR is opened. Please list your opinions.
Summary by CodeRabbit