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

Adding HLG to MAP #8740

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Adding HLG to MAP #8740

wants to merge 17 commits into from

Conversation

alex-rakowski
Copy link
Contributor

@alex-rakowski alex-rakowski commented Jun 28, 2024

Closes #8706

  • Tests added / passed
  • Passes pre-commit run --all-files

@alex-rakowski alex-rakowski changed the title first pass adding HLG dding HLG to MAP Jun 28, 2024
@alex-rakowski alex-rakowski changed the title dding HLG to MAP Adding HLG to MAP Jun 28, 2024
@hendrikmakait hendrikmakait self-requested a review June 28, 2024 13:17
Copy link
Contributor

github-actions bot commented Jun 28, 2024

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

    29 files  ±0      29 suites  ±0   11h 18m 6s ⏱️ - 1m 50s
 4 083 tests ±0   3 967 ✅ +3    112 💤 ±0  4 ❌  - 3 
55 230 runs  ±0  52 791 ✅ +3  2 433 💤  - 1  6 ❌  - 2 

For more details on these failures, see this check.

Results for commit 2eb19b7. ± Comparison against base commit 670fbfb.

♻️ This comment has been updated with latest results.

distributed/client.py Outdated Show resolved Hide resolved
distributed/client.py Outdated Show resolved Hide resolved
The MapLayer constructor has been refactored to handle nested iterables. This allows for more flexibility when passing in iterables to the MapLayer class. The changes include:
- Checking if the iterables are nested and converting them into a list of tuples if necessary.
- Updating the logic for generating keys based on the nested iterables.
- Modifying the construction of the low-level graph to handle the nested iterables.

These changes improve the usability and versatility of the MapLayer class.
@alex-rakowski alex-rakowski marked this pull request as ready for review July 4, 2024 16:38
Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

Codecov is very unhappy, this could be related to this error: https://github.com/dask/distributed/actions/runs/9828037658/job/27131436761?pr=8740#step:20:76

@@ -807,6 +809,116 @@ class VersionsDict(TypedDict):
client: dict[str, dict[str, Any]]


_T_LowLevelGraph: TypeAlias = dict[Key, tuple]
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope: We should probably move this into dask.typing now that we've added it in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/alex-rakowski/dask/blob/c619f1da760fc20af33d67c72cec24e9c5d7a58d/dask/typing.py#L30-L31

would that be adding here? There is a FIXME here, seems like we'd want to add a _T_LowLevelGraph and _T_HighLevelGraph types?

Copy link
Member

Choose a reason for hiding this comment

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

This should match the dask.typing.Graph type but I'm not very keen of this type alias. I suggest to not do anything for now

distributed/client.py Outdated Show resolved Hide resolved
distributed/client.py Outdated Show resolved Hide resolved
distributed/client.py Outdated Show resolved Hide resolved
distributed/client.py Outdated Show resolved Hide resolved
distributed/client.py Outdated Show resolved Hide resolved
distributed/tests/test_client.py Show resolved Hide resolved
@@ -807,6 +809,116 @@ class VersionsDict(TypedDict):
client: dict[str, dict[str, Any]]


_T_LowLevelGraph: TypeAlias = dict[Key, tuple]
Copy link
Member

Choose a reason for hiding this comment

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

This should match the dask.typing.Graph type but I'm not very keen of this type alias. I suggest to not do anything for now

distributed/client.py Outdated Show resolved Hide resolved
distributed/client.py Outdated Show resolved Hide resolved
distributed/client.py Show resolved Hide resolved
distributed/client.py Show resolved Hide resolved
distributed/client.py Outdated Show resolved Hide resolved
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.

Client.map should be using a HLG Layer
3 participants