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

Add custom prefix to all HTTP server URLs #1338

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

Conversation

confluence
Copy link
Collaborator

@confluence confluence commented Nov 28, 2023

Description

This is a re-creation of #1307 with a copy of the branch in our main repo, and implements #1309.

To be done before this can move out of draft:

Checklist

  • changelog updated / no changelog update needed
  • e2e test passing / corresponding fix added / new e2e test created
  • protobuf updated to the latest dev commit / no protobuf update needed
  • protobuf version bumped / protobuf version not bumped
  • added reviewers and assignee

@confluence confluence self-assigned this Nov 28, 2023
@confluence confluence mentioned this pull request Nov 28, 2023
5 tasks
@confluence confluence marked this pull request as draft November 28, 2023 14:46
@confluence confluence added the awaiting code changes For pull requests that require code changes label Nov 29, 2023
@kswang1029 kswang1029 added this to the v5.0-beta milestone Dec 14, 2023
…level string processing with regexes. Revise normalization logic. Add the prefix to the api address reported to the frontend.
@confluence confluence added awaiting code review For pull requests that require code review awaiting testing For pull requests that require testing and removed awaiting code changes For pull requests that require code changes labels May 8, 2024
@confluence confluence requested a review from daikema May 8, 2024 18:21
@confluence confluence marked this pull request as ready for review May 8, 2024 18:22
@confluence
Copy link
Collaborator Author

@appolloford and @Micket, does this solution still work for you with the prefix set using a configuration option rather than an environment variable? This option can be passed on the commandline (carta_backend --http_url_prefix /foo/bar ...), or set globally or per-user in one of the config files.

@appolloford
Copy link

Having a configuration option in commandline should work well, but I am wondering whether the priority is higher than users' config file. We need the node name and port number information in the URL prefix so that we can have OnDemand find the computing node. There may be some trouble if users overwrite it with some arbitrary value.

@confluence
Copy link
Collaborator Author

@appolloford yes, the commandline parameters have the highest priority, and override the user config file, which overrides the global config file. You can also use commandline parameters to disable the user and/or global config file entirely.

@kswang1029
Copy link
Contributor

@confluence do we consider the following use cases?

url_without_token = 'http://localhost:3003'


session = Session.start_and_create(Chrome(headless=True, options=custom_chrome_options), "/Users/kswang/carta_build/carta-backend/build/carta_backend", params=("/Users/kswang/set_QA_e2e_v2", "--frontend_folder", "/Users/kswang/carta_build/carta-frontend/build", "--port", "3003", "--debug_no_auth", "--http_url_prefix", "/foo/bar"))

If I do so, I see errors as

Traceback (most recent call last):
  File "/Users/kswang/carta_python_e2e_test/test.py", line 12, in <module>
    session = Session.start_and_create(Chrome(headless=True, options=custom_chrome_options), "/Users/kswang/carta_build/carta-backend/build/carta_backend", params=("/Users/kswang/set_QA_e2e_v2", "--frontend_folder", "/Users/kswang/carta_build/carta-frontend/build", "--port", "3003", "--debug_no_auth", "--http_url_prefix", "/foo/bar"))
  File "/Users/kswang/anaconda3/lib/python3.10/site-packages/carta/session.py", line 224, in start_and_create
    return browser.new_session_with_backend(executable_path, remote_host, params, timeout, token, frontend_url_timeout)
  File "/Users/kswang/anaconda3/lib/python3.10/site-packages/carta/browser.py", line 144, in new_session_with_backend
    return self.new_session_from_url(backend.frontend_url, backend.token, backend=backend, timeout=timeout, debug_no_auth=backend.debug_no_auth)
  File "/Users/kswang/anaconda3/lib/python3.10/site-packages/carta/browser.py", line 98, in new_session_from_url
    self.exit(f"Could not parse session ID from frontend. Last error: {last_error}")
  File "/Users/kswang/anaconda3/lib/python3.10/site-packages/carta/browser.py", line 149, in exit
    raise CartaBadSession(msg)
carta.util.CartaBadSession: Could not parse session ID from frontend. Last error: Message: no such element: Unable to locate element: {"method":"css selector","selector":"[id="info-session-id"]"}
  (Session info: chrome-headless-shell=125.0.6422.141)
...

The core question is how do we support the scripting interface when the --http_url_prefix flag is set? Or this is not a problem as that flag is used and set by remote server manager, so that we (the client) should just set the URL as seen with the custom prefix directly?

@confluence
Copy link
Collaborator Author

@kswang1029 that's a wrapper issue which should be discussed here.

I thought that the wrapper didn't require changes, but I hadn't fully tested it yet.

@confluence
Copy link
Collaborator Author

@kswang1029 actually, this is an issue with debug_no_auth here in the backend code -- I can reproduce it without the wrapper. (I'm also cleaning up some URL parsing code in the wrapper, but it's not related to this issue). I will investigate further.

Copy link

github-actions bot commented Jun 3, 2024

Code Coverage

Package Line Rate Health
src.Cache 72%
src.DataStream 44%
src.FileList 67%
src.Frame 36%
src.HttpServer 42%
src.ImageData 28%
src.ImageFitter 83%
src.ImageGenerators 43%
src.ImageStats 75%
src.Logger 37%
src.Main 52%
src.Region 69%
src.Session 4%
src.Table 52%
src.ThreadingManager 67%
src.Timer 85%
src.Util 40%
Summary 46% (8621 / 18816)

@confluence
Copy link
Collaborator Author

@kswang1029 I have pushed a change which I think fixes this issue. With this fix, I believe that your use case should work with the current development wrapper. Additionally, wrapper PR #154 fixes a parsing issue which breaks starting a backend from the wrapper with a prefix and auth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting code review For pull requests that require code review awaiting testing For pull requests that require testing
Projects
No open projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants