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

feat: Allow user-specified local dir to be served by Tornado #1428

Closed
wants to merge 1 commit into from

Conversation

rht
Copy link
Contributor

@rht rht commented Sep 30, 2022

No description provided.

local_dirs.append(local_dir) # Set this to be the last one.
for _local_dir in local_dirs:
local_handler = (
r"/local/(.*)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to check with Tornado doc on whether this reusing of the pattern is even allowed.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also wondering what'll happen if two origin folders with the same structure get mapped to the same destination folder.

Mesa-Geo has a js folder and a css folder under templates folder that maps to /local. Perhaps Mesa-Geo users also define their own frontend elements in the same way. Then we have another set of js and css folders mapped to /local. Not sure whether this will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found https://groups.google.com/g/python-tornado/c/xYp6DJTlCsw. One suggestion was to define a multi file handler custom class.

Aside, I suppose the only way to know that it is allowed is to test it. Since it works on Mesa-Geo, does that mean it is fine?

Copy link
Member

Choose a reason for hiding this comment

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

It is fine most likely due to there is only one defined local_dir from Mesa-Geo. I did some tests with my https://github.com/wang-boyu/agents-and-networks-in-python example. Initially it also works fine, when I use your branch and mine.

There is an issue when I define a custom frontend module in the example, using the same folder structure. Something like

class CustomFrontendModule(VisualizationElement):
    local_includes = [
        "js/CustomFrontendModule.js",
        "css/external/test.css",
        "js/external/test.js",
    ]
    local_dir = os.path.dirname(__file__) + "/../templates"

    def __init__(self):
        super().__init__()

Then I use the new element together with that from Mesa-Geo. If I put it after Mesa-Geo's MapModule like this:

    map_element = MapModule(
        agent_draw, **map_params[args.campus], map_height=600, map_width=600
    )
    custom_element = CustomFrontendModule()
    server = ModularServer(
        AgentsAndNetworks,
        [map_element, custom_element, clock_element, status_chart, friendship_chart],
        "Agents and Networks",
        model_params=model_params,
    )

then the map gets loaded properly and I get the following messages from the command line:

404 GET /local/css/external/test.css (127.0.0.1) 2.49ms
404 GET /local/js/CustomFrontendModule.js (127.0.0.1) 3.43ms
404 GET /local/js/external/test.js (127.0.0.1) 2.42ms

If I swap them, i.e.,

server = ModularServer(
        AgentsAndNetworks,
-        [map_element, custom_element, clock_element, status_chart, friendship_chart],
+        [custom_element, map_element, clock_element, status_chart, friendship_chart],
        "Agents and Networks",
        model_params=model_params,
    )

then the map is gone and I get the following:

404 GET /local/css/external/leaflet.css (127.0.0.1) 1.70ms
404 GET /local/js/MapModule.js (127.0.0.1) 2.65ms
404 GET /local/js/external/leaflet.js (127.0.0.1) 1.27ms

So it seems that /local gets mapped on a first-come-first-served basis.

Perhaps the MultiStaticFileHandler worth a try? But if there are multiple files from different local_dir with the same name, I guess the first one gets found and loaded.

Another more complex implementation is we somehow keep track of which package each local_dir comes from, and create dedicated folder accordingly. For instance we can have something like:

local
├── mesa_geo
│   ├── js
│   │   └── index.js
│   └── css
└── agents-and-networks
    ├── js
    │   └── index.js
    └── css

We also need to be able to load files from their folders only, so that the two index.js get loaded from their own packages correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternative (hopefully simpler) is to have a static file handler that serves from a Python string instead.

local_includes = ["local/my_custom_path/script.js"]
local_handler = (
    r"/local/my_custom_path/script.js",
    tornado.web.StaticFileHandler,
    {"file_content": the_file_content_str},
)

Copy link
Contributor Author

@rht rht Oct 1, 2022

Choose a reason for hiding this comment

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

I can't test this idea easily, but maybe you can. Here is a sketch:

  1. ask the custom viz author to specify a dictionary of path -> string_content
custom_handlers = []
for path, content in custom_contents.items():
    class MyHandler(tornado.web.RequestHandler):
        def get(self):
            return content
    custom_handlers.append(("/" + path, MyHandler))
self.handlers += custom_handlers

Copy link
Member

Choose a reason for hiding this comment

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

Hmm interesting. Let me see what I can do.

Copy link
Member

Choose a reason for hiding this comment

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

I made some simple modifications to your code, to put package-specific files into their own directory under /local. The directory name is currently set to be the class name, but we can change it if you prefer something else. The resulting directory looks like this:

local
├── MapModule  # <------------- from mesa-geo
│   ├── js
│   │   └── index.js
│   └── css
└── CustomFrontendModule  # <-- from agents-and-networks example
    ├── js
    │   └── index.js
    └── css

From the user side, it's the same as last time:

class MapModule(VisualizationElement):
    local_includes = [
        "js/MapModule.js",
        "css/external/leaflet.css",
        "js/external/leaflet.js",
    ]
    local_dir = os.path.dirname(__file__) + "/../templates"

If the user omits local_dir (such as in the examples) then the default is "".

My code is at wang-boyu@d1452e2. I can create a PR to your branch if you'd like to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that looks like another clean solution. I think you can make a PR to projectmesa/mesa straight away, to make the process simpler.

Copy link
Member

Choose a reason for hiding this comment

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

Created #1435.

@codecov
Copy link

codecov bot commented Sep 30, 2022

Codecov Report

Base: 90.85% // Head: 90.76% // Decreases project coverage by -0.09% ⚠️

Coverage data is based on head (2a24e8d) compared to base (ecb7c91).
Patch coverage: 80.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1428      +/-   ##
==========================================
- Coverage   90.85%   90.76%   -0.10%     
==========================================
  Files          15       15              
  Lines        1291     1299       +8     
  Branches      256      219      -37     
==========================================
+ Hits         1173     1179       +6     
- Misses         83       84       +1     
- Partials       35       36       +1     
Impacted Files Coverage Δ
mesa/visualization/ModularVisualization.py 77.91% <80.00%> (-0.16%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -328,6 +327,9 @@ def __init__(
self.local_css_includes.add(include_file)
else:
self.local_js_includes.add(include_file)
if len(element.local_includes) > 0:
if element.local_dir != "":
local_dirs.append(element.local_dir)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wang-boyu let me know if this construct is unnecessary, and I will just simplify this PR to be just using 1 local_dir.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably necessary? We may have multiple local_includes from different extensions and users.

@rht
Copy link
Contributor Author

rht commented Oct 4, 2022

Superseded by #1435

@rht rht closed this Oct 4, 2022
@rht rht deleted the modularserver_ext branch October 4, 2022 02:54
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.

2 participants