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

Added explicit pathwaysutils.initialize() call to JetStream #233

Merged
merged 1 commit into from
Apr 8, 2025

Conversation

wstcliyu
Copy link
Collaborator

@wstcliyu wstcliyu commented Apr 2, 2025

Description

pathwaysutils will soon require a call to initialize().

Tests

Depending on GitHub actions for tests

Checklist

Before submitting this PR, please make sure (put X in square brackets):

  • I have performed a self-review of my code.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have run end-to-end tests tests and provided workload links above if applicable.
  • I have made or will make corresponding changes to the doc if needed.

Copy link
Collaborator

@vivianrwu vivianrwu left a comment

Choose a reason for hiding this comment

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

I think this is less of an "error" and more of "we are running jetstream without pathways. Could we clarify too that this could be intended behavior, or else users may think that there is some failure happening.

@wstcliyu wstcliyu force-pushed the wstcliyu/pw-util-init branch from 2fba2da to ddf43ec Compare April 3, 2025 18:31
@wstcliyu
Copy link
Collaborator Author

wstcliyu commented Apr 3, 2025

I think this is less of an "error" and more of "we are running jetstream without pathways. Could we clarify too that this could be intended behavior, or else users may think that there is some failure happening.

Thanks. Updated the log to "Running JetStream without Pathways. Module pathwaysutils is not imported." Please review again.

Copy link
Collaborator

@vipannalla vipannalla left a comment

Choose a reason for hiding this comment

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

looks good, can you fix the lint issue?

Copy link
Collaborator

@vivianrwu vivianrwu left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, lgtm!

@wstcliyu wstcliyu force-pushed the wstcliyu/pw-util-init branch from ddf43ec to 8d385b6 Compare April 3, 2025 22:05
@wstcliyu
Copy link
Collaborator Author

wstcliyu commented Apr 3, 2025

It's weird that it could not pass the checks (pyink format check & test coverage report). I don't see a quick fix due to lack of change suggestions. Shall we bypass the checks?

@vivianrwu
Copy link
Collaborator

It's weird that it could not pass the checks (pyink format check & test coverage report). I don't see a quick fix due to lack of change suggestions. Shall we bypass the checks?

pyink you can fix by running pyink --pyink-indentation 2 --line-length 80 <file_name>

@wstcliyu wstcliyu force-pushed the wstcliyu/pw-util-init branch from 8d385b6 to 1e2dfad Compare April 3, 2025 22:19
@wstcliyu
Copy link
Collaborator Author

wstcliyu commented Apr 3, 2025

Thank you. Could you also guide me how to add test coverage for pathwaysutils.initialize(). pathwaysutils is not in the requirements file so this line is never called really.

@vivianrwu
Copy link
Collaborator

Thank you. Could you also guide me how to add test coverage for pathwaysutils.initialize(). pathwaysutils is not in the requirements file so this line is never called really.

You could probably write a test expecting the error.

@wstcliyu wstcliyu force-pushed the wstcliyu/pw-util-init branch 4 times, most recently from b03b5c6 to b1a70eb Compare April 4, 2025 21:30
@wstcliyu
Copy link
Collaborator Author

wstcliyu commented Apr 4, 2025

@vipannalla I added the unit test for module engine init. This unit test can pass by running separately coverage run -m unittest -v jetstream.tests.engine.test_init but cannot pass by coverage run -m unittest -v

@wstcliyu wstcliyu force-pushed the wstcliyu/pw-util-init branch 2 times, most recently from 9fa4fb8 to 596019c Compare April 7, 2025 23:32
@wstcliyu wstcliyu force-pushed the wstcliyu/pw-util-init branch from 596019c to b8c1b7d Compare April 7, 2025 23:41
@wstcliyu wstcliyu force-pushed the wstcliyu/pw-util-init branch from b8c1b7d to 46247f0 Compare April 7, 2025 23:44
@vipannalla
Copy link
Collaborator

Its complaining about unit test coverage for a test file, which should really be excluded from the check.

@vipannalla vipannalla merged commit 8aa6a9e into main Apr 8, 2025
2 of 3 checks passed
@vipannalla vipannalla deleted the wstcliyu/pw-util-init branch April 8, 2025 01:11
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