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

Fix/empty dict to dependencies indicating all drivers start concurrently #1103

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

zhenyu-ms
Copy link
Contributor

@zhenyu-ms zhenyu-ms commented Jun 18, 2024

Bug / Requirement Description

Clearly and concisely describe the problem.

Solution description

Describe your code changes in detail for reviewers.

Checklist:

  • Test
  • Example (both test_plan.py and .rst)
  • Documentation (API)
  • News fragment present for release notes
  • MS info leakage check
  • For new driver: driver index page
  • For new assertion: ui/pdf/std renderers, documentation
  • For new cmdline arg: documentation

@zhenyu-ms zhenyu-ms requested a review from a team as a code owner June 18, 2024 11:08
@@ -112,7 +112,7 @@ def get_options(cls):
[Or(Resource, RemoteDriver)], validate_func()
),
ConfigOption("dependencies", default=None): Or(
Use(parse_dependency), validate_func()
None, Use(parse_dependency), validate_func()
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest we get rid of dependencies=None usage
it easily confuses with dependencies=[]

else:
deps = self.cfg.dependencies
if deps:
if deps is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do a None check in set_dependency and make this piece of code simpler?

@@ -0,0 +1 @@
Users now can make all drivers of a certain ``Test`` start concurrently by passing an empty dict to ``dependencies`` argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

update the dependencies arg doc

dependencies=dependencies,
initial_context={"is_set": is_set},
environment=lambda: list(drivers.values())
if is_lift
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this param named is_lift, shall we call it use_callable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😬

@zhenyu-ms zhenyu-ms changed the title Fix/empty dict passed to dependencies means all drivers in parallel Fix/empty dict passed to dependencies means all drivers concurrently Jun 20, 2024
@zhenyu-ms zhenyu-ms changed the title Fix/empty dict passed to dependencies means all drivers concurrently Fix/empty dict to dependencies indicating all drivers start concurrently Jun 20, 2024
@zhenyu-ms zhenyu-ms merged commit e47fba0 into morganstanley:main Jun 20, 2024
15 checks passed
@zhenyu-ms zhenyu-ms deleted the fix-driver-dep branch July 16, 2024 06:31
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