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

Prototype ray task #29

Draft
wants to merge 9 commits into
base: development
Choose a base branch
from
Draft

Prototype ray task #29

wants to merge 9 commits into from

Conversation

dangsg
Copy link
Collaborator

@dangsg dangsg commented Jan 8, 2025

No description provided.

@dangsg dangsg requested a review from ake2l January 8, 2025 06:31
@dangsg dangsg marked this pull request as draft January 8, 2025 06:32
@dangsg dangsg self-assigned this Jan 8, 2025
@dangsg dangsg marked this pull request as ready for review January 8, 2025 06:37
@ake2l ake2l marked this pull request as draft January 10, 2025 09:48
Copy link
Member

@ake2l ake2l left a comment

Choose a reason for hiding this comment

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

looking forward to your next draft and investigation results

return TaskUtil.evaluate_variable_concat_prefix_suffix(context, selector, prefix=prefix, suffix=suffix)

@staticmethod
def _load_csv_file(
Copy link
Member

Choose a reason for hiding this comment

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

for consistency remove _ ... load_csv_file instead for _load_csv_file

@@ -241,6 +241,7 @@ def test_calculate_default_page_size_with_statement_page_size(self, generate_tas
assert size == 100
assert mock_statement.page_size == 100

@pytest.mark.skip("Need rework with ray")
Copy link
Member

Choose a reason for hiding this comment

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

before PR is done needs to be reworked



def _geniter_single_process_generate(args: tuple) -> dict[str, list]:
def _geniter_single_process_generate(context: SetupContext | GenIterContext, stmt: GenerateStatement, page_start: int,
Copy link
Member

Choose a reason for hiding this comment

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

should not exists anymore ... and we should re-evaluate the naming of the function to make it more clear what they are actually doing

page_index == len(index_chunk) - 1,
)
# Determine number of Ray workers
num_workers = int(self.statement.num_process or context.root.num_process or 1)
Copy link
Member

Choose a reason for hiding this comment

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

yeah ... this is how it should be ... default is num_process = 1 ... we need to make sure the ray framework is doing what we expect ... last time even when i specified 1 process ... many ray processes were created without any load ... haven't investigated more if this behavior is normal and how to address

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