Skip to content

[Bug] avoid frequent lookup of the routing strategy env #956

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

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

Iceber
Copy link
Contributor

@Iceber Iceber commented Apr 8, 2025

Pull Request Description

[Please provide a clear and concise description of your changes here]

There's no need to check the environment variables every time a request is processed.

Related Issues

Resolves: #[Insert issue number(s)]

Important: Before submitting, please complete the description above and review the checklist below.


Contribution Guidelines (Expand for Details)

We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:

Pull Request Title Format

Your PR title should start with one of these prefixes to indicate the nature of the change:

  • [Bug]: Corrections to existing functionality
  • [CI]: Changes to build process or CI pipeline
  • [Docs]: Updates or additions to documentation
  • [API]: Modifications to aibrix's API or interface
  • [CLI]: Changes or additions to the Command Line Interface
  • [Misc]: For changes not covered above (use sparingly)

Note: For changes spanning multiple categories, use multiple prefixes in order of importance.

Submission Checklist

  • PR title includes appropriate prefix(es)
  • Changes are clearly explained in the PR description
  • New and existing tests pass successfully
  • Code adheres to project style and best practices
  • Documentation updated to reflect changes (if applicable)
  • Thorough testing completed, no regressions introduced

By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.

@@ -50,30 +50,20 @@ func validateStreamOptions(requestID string, user utils.User, jsonMap map[string
return nil
}

var defaultRoutingStrategy, defaultRoutingStrategyEnabled = utils.CheckEnvExists(EnvRoutingAlgorithm)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CheckEnvExists doesn't seem like the best name. Maybe rename it to LookupEnv or just use os.LookupEnv directly?

// CheckEnvExists checks if an environment variable exists.
// It returns the value and a boolean indicating its existence.
func CheckEnvExists(envVar string) (string, bool) {
value, exists := os.LookupEnv(envVar)
return value, exists
}

@Jeffwan What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, rename it to LookupEnv.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good.

@Iceber
Copy link
Contributor Author

Iceber commented Apr 8, 2025

The failed ci is not related to the modification of this pr.

@Jeffwan
Copy link
Collaborator

Jeffwan commented Apr 8, 2025

image

It is a separate issue. I will take a look at the dependency, that's the object storage in volcano engine cloud

@varungup90
Copy link
Collaborator

Overall look good.

@Jeffwan
Copy link
Collaborator

Jeffwan commented Apr 8, 2025

it's a little bit weird that build jobs are either canceled or skipped..

@Iceber Iceber force-pushed the get_routing_strategy branch from 49ee732 to 3b016de Compare April 9, 2025 02:44
@Iceber Iceber force-pushed the get_routing_strategy branch from 3b016de to f3bd5c8 Compare April 9, 2025 02:45
@varungup90 varungup90 merged commit e450f5d into vllm-project:main Apr 9, 2025
11 checks passed
@Jeffwan
Copy link
Collaborator

Jeffwan commented Apr 9, 2025

@varungup90 Did the PR pass the CI earlier or just skipped the test? the main branch test failed again.

image

@varungup90
Copy link
Collaborator

@varungup90 Did the PR pass the CI earlier or just skipped the test? the main branch test failed again.

image

First time when you commented on the PR for CI failures, at that time I manually restarted the failed jobs and they succeeded. When the PR was updated with method rename, all CI job succeeded so I merged (initially also failures were not related to PR). After PR merged, there were failures again that you brought to my attention.

@Jeffwan
Copy link
Collaborator

Jeffwan commented Apr 9, 2025

@varungup90 yeah, this is kind of weird, all parallel CI jobs yesterday pass and seems my current fix doesn't help as well. I tried to build locally but I can not reproduce the issue, probably due to some arch diffs. I will spend some time later on this.

If you did see they succeeded, it's probably due to other cache issues.

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.

3 participants