-
Notifications
You must be signed in to change notification settings - Fork 21
[FIX] Fixed zoom in zoom out and Block dynamic effects #132
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
base: main
Are you sure you want to change the base?
[FIX] Fixed zoom in zoom out and Block dynamic effects #132
Conversation
… fix MANIFEST EOF
@gujialiang123 please resolve merge conflicts |
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
from triton_viz.visualizer.interface import stop_server, launch | ||
|
||
stop_server() | ||
launch(share=False, port=5001) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Remove implicit server launch on import
Importing triton_viz.core.trace
now pulls in the Flask visualizer and immediately calls stop_server()
and launch(share=False, port=5001)
. Any consumer that simply imports the tracing API will spawn a Flask thread and bind port 5001, which fails if the port is busy and can conflict with a later explicit triton_viz.launch()
call. Module imports should remain side‑effect free; starting the server should be an explicit user action.
Useful? React with 👍 / 👎.
# and can be added to the global gitignore or merged into this file. For a more nuclear | ||
# option (not recommended) you can uncomment the following to ignore the entire idea folder. | ||
#.idea/ | ||
Triton-Puzzles20250829.ipynb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take it out from .gitignore
@@ -0,0 +1,2 @@ | |||
recursive-include triton_viz/templates *.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file necessary?
include-package-data = true | ||
|
||
[tool.setuptools.package-data] | ||
"triton_viz" = ["templates/*.html", "static/*.js", "static/**/*.js", "static/**"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have already specified files here, so I don't think MANIFEST.in
is necessary
try: | ||
import sys as _sys | ||
|
||
for name, mod in list(_sys.modules.items()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good way to access launches. Better to modify the import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or pass launches to collect grid
all_grids.setdefault(current_idx, []) | ||
# append non-Grid ops | ||
all_grids[current_idx].append(r) | ||
if _is_type(r, "OutOfBoundsRecordBruteForce"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand why we need this method _is_type
# Implement a way to stop the Flask server | ||
pass | ||
|
||
def get_last_public_url(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to put last_public_url
and last_public_port
into a struct for access. Now that you define get_last_public_url
but never use it
print(f"Running on local URL: {local_url}") | ||
print("--------") | ||
app.run(port=5001) | ||
global last_local_port |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not good practice
except requests.exceptions.RequestException: | ||
print("Setting up public URL... Please wait.") | ||
# Even if the readiness check fails, return the intended URLs so callers don't crash | ||
local_url = f"http://localhost:{actual_port}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is duplicated with line 244-250
Fixed ##125 and The animation effect of the visualizer is working now
