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

[style] flake8 violation E501 fix #48099

Open
wants to merge 242 commits into
base: master
Choose a base branch
from

Conversation

JP-sDEV
Copy link

@JP-sDEV JP-sDEV commented Oct 18, 2024

Why are these changes needed?

Fixes flake8 linting error E501, line too long (82 > 79 characters).

Ran the flake8 linting script

flake8 --select E501 --extend-exclude python/ray/core/generated,python/ray/serve/generated/,python/ray/cloudpickle/,python/ray/_private/runtime_env/_clonevirtualenv.py,doc/external/,python/ray/dashboard/client/node_modules,python/.venv

Note: python/.venv was ignored added to the original script since I created a virtualenv to run the flake8 script.

Related issue number

Closes #48061

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

cc: @MortalHappiness

@MortalHappiness
Copy link
Member

Hi @JP-sDEV, thank you for your contribution! However, you forgot to sign off on your latest commit, and the lint CI failed. Please note that the pre-commit hook still needs to be run, as I mentioned in the parent issue. Make sure the flake8 command passes before you create a commit.

@JP-sDEV
Copy link
Author

JP-sDEV commented Oct 20, 2024

Sure, I will make sure the flake8 command passes as well as the lint workflow passes in my forked repo passes before resubmitting. Will start working on it on Monday!

Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>
Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>
Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>
python/ray/workflow/http_event_provider.py Outdated Show resolved Hide resolved
python/ray/tune/tests/test_progress_reporter.py Outdated Show resolved Hide resolved
python/ray/tune/tests/test_progress_reporter.py Outdated Show resolved Hide resolved
Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>
@JP-sDEV
Copy link
Author

JP-sDEV commented Oct 22, 2024

Thank you @MortalHappiness, the suggested changes have been applied. Cheers! :)

python/ray/workflow/http_event_provider.py Outdated Show resolved Hide resolved
python/ray/tune/tests/test_progress_reporter.py Outdated Show resolved Hide resolved
python/ray/tune/tests/test_progress_reporter.py Outdated Show resolved Hide resolved
python/ray/tune/tests/test_progress_reporter.py Outdated Show resolved Hide resolved
python/ray/tune/tests/test_progress_reporter.py Outdated Show resolved Hide resolved
python/ray/tune/tests/test_progress_reporter.py Outdated Show resolved Hide resolved
Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>
@JP-sDEV
Copy link
Author

JP-sDEV commented Oct 23, 2024

@MortalHappiness done! Thank you for your patience and for guiding me through this :)

Signed-off-by: JP-sDEV <jon.pablo80@gmail.com>
@MortalHappiness
Copy link
Member

@JP-sDEV Could you rebase with the master branch to trigger the CI to see if the failed tests can pass? Thanks

@jcotant1
Copy link
Collaborator

Hey @JP-sDEV @MortalHappiness what's the status of this one?

@MortalHappiness
Copy link
Member

@jcotant1 I am waiting for @JP-sDEV to sync with the master branch.

@JP-sDEV
Copy link
Author

JP-sDEV commented Nov 14, 2024

@MortalHappiness @jcotant1 working on it now, should be pushed in a couple hours. Apologies for the delay

omatthew98 and others added 2 commits November 14, 2024 09:02
…-project#48093)

## Why are these changes needed?
We currently have our default logging configuration as a YAML file that
we then load as a python dictionary. This forces us to include the yaml
in the ray wheel. This inlines the configuration into a python
dictionary to be included in `logging.py`.

## Related issue number

## Checks

- [x] I've signed off every commit(by using the -s flag, i.e., `git
commit -s`) in this PR.
- [x] I've run `scripts/format.sh` to lint the changes in this PR.
- [ ] I've included any doc changes needed for
https://docs.ray.io/en/master/.
- [ ] I've added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [ ] I've made sure the tests are passing. Note that there might be a
few flaky tests, see the recent failures at https://flakey-tests.ray.io/
- Testing Strategy
   - [x] Unit tests
   - [ ] Release tests
   - [ ] This PR is not tested :(

Signed-off-by: Matthew Owen <mowen@anyscale.com>
…led Test for Windows (ray-project#48054)

* Resubmit ray-project#47871 and ray-project#48030 as they are reverted as release blocker
* Fixed the failed test in windows CI test. 
* The test `test_metrics_folder_and_content` failed because of the
different file separator between linux and Windows
* The PR fix the issue by using `os.path.join` to generate the path for
validation instead of directly using formatted string

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>
dentiny and others added 10 commits November 14, 2024 09:04
Some minor improvements for session implementation.

Signed-off-by: dentiny <dentinyhao@gmail.com>
## Why are these changes needed?

- Combine the endpoint router and the longest prefix router. The two
differed in that one used `match_route` to get a handle, and one used
`get_handle_for_endpoint` to get a handle (for HTTP and gRPC
respectively). We can just have a single proxy router class that have
both of these methods.
  - This is nice in that we only have one cached set of handles.
- Add a default `get_proxy_handle` function in `default_impl.py`, which
can be used to control how a handle is fetched and how it is
initialized.
- Infer request protocol from request context, which should
deterministically indicate whether the request is an http request or a
gRPC request. This then removes the need to set request protocol in the
handle, which makes sense since request protocol is more metadata than
handle configuration.


Signed-off-by: Cindy Zhang <cindyzyx9@gmail.com>
…)" (ray-project#48612)" (ray-project#48686)

This reverts commit 4a9c424.

---------

Signed-off-by: Balaji Veeramani <bveeramani@berkeley.edu>
https://github.com/ray-project/ray/pull/48693/files#diff-81d7373cd5567e997c002b244c491e6b3498d206b12c093f4dc4d30e9b5848af
added a test that uses tensorflow. We currently need to skip all
tensorflow-related tests for python 3.12 since we don't support
tensorflow for python 3.12.

Also this is a test for the deprecation of tensorflow ;)

Test:
- CI

Signed-off-by: can <can@anyscale.com>
…tensors > 2Gb) (ray-project#48629)

Enabling V2 Arrow Tensor extension type by default (allowing tensors >
2Gb)

---------

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>
…Error (ray-project#48636)

In a recent investigation, we found that when we call the
`ray._private.internal_api.free()` from a task the same time as a Raylet
is gracefully shutting down, the task might fail with application level
Broken pipe IOError. This resulted in job failure without any task
retries.

However, as the Broken pipe happens because the unhealthiness of the
local Raylet, the error should be a system level error and should be
retried automatically.

Updated changes in commit
[01f5f11](ray-project@01f5f11):
This PR add the logic for the above bahvior:
* When IOError is received in the `CoreWorker::Delete`, throw a system
error exception so that the task can retry

Why not add the exception check in the `free_objects` function?
* It is better to add the logic in the `CoreWorker::Delete` because it
can cover the case for other languages as well.
* The `CoreWorker::Delete` function is intended to be open to all
languages to call and is not called in other ray internal code paths.

Why not crash the worker when IOError is encountered in the
`WriteMessage` function?
* `QuickExit()` function will directly exit the process without
executing any shutdown logic for the worker. Directly calling the
function in the task execution might potentially causing resource leak
* At the same time, the write message function is called also on the
graceful shutdown scenario and it is possible during the graceful
shutdown process that the local Raylet is unreachable. Therefore, in the
graceful shutdown scenario, we shouldn't exit early but let the shutdown
logic finish.
* At the same time, it is not clear in the code regarding the behavior
of the graceful vs force shutdown. We might need some effort to make
them clear. The todo is added in the PR.

Updated changes in commit
[2029d36](ray-project@2029d36):
> This PR add the logic for the above behavior:
> * When IOError is received in the `free_objects()` function, throw a
system error exception so that the task can retry

Changes in commit
([9d57b29](ray-project@9d57b29))
:
> This PR add the logic for the above behavior:
> * Today, the internal `free` API deletes the objects from the local
Raylet object store by writing a message through a socket
> * When the write failed because the local Raylet is terminated, there
is already logic to quick exit the task
> * However, the current termination check didn't cover the case where
the local Raylet process is a Zombie process and IOError happens during
write messages.
> * This fix update the check criteria and fail the task when the Raylet
process is terminated or the write message function returns an IOError~


Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>
## Why are these changes needed?
While we calling `xxx.map_groups(..., batch_format="...")`, we may
invoke sort function and creating empty blocks which still uses pyarrow
by default. And, when we invoke another sort call on top of it, we will
hit `AttributeError: 'DataFrame' object has no attribute 'num_rows'`
since we uses first block type. (However, we may have different blocks).
See more details in ray-project#46748

## Related issue number

Close ray-project#46748

## Checks

- [x] I've signed off every commit(by using the -s flag, i.e., `git
commit -s`) in this PR.
- [x] I've run `scripts/format.sh` to lint the changes in this PR.
- [ ] I've included any doc changes needed for
https://docs.ray.io/en/master/.
- [ ] I've added any new APIs to the API Reference. For example, if I
added a
method in Tune, I've added it in `doc/source/tune/api/` under the
           corresponding `.rst` file.
- [x] I've made sure the tests are passing. Note that there might be a
few flaky tests, see the recent failures at https://flakey-tests.ray.io/
- Testing Strategy
   - [x] Unit tests
   - [ ] Release tests
   - [ ] This PR is not tested :(

---------

Signed-off-by: Xingyu Long <xingyulong97@gmail.com>
Co-authored-by: Scott Lee <scottjlee@users.noreply.github.com>
…oject#48729)

Created by release automation bot.

Update with commit e393a71

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>
Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com>
…es (ray-project#48478)

<!-- Thank you for your contribution! Please review
https://github.com/ray-project/ray/blob/master/CONTRIBUTING.rst before
opening a pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

When writing blocks to parquet, there might be blocks with fields that
differ ONLY in nullability - by default, this would be rejected since
some blocks might have a different schema than the ParquetWriter.
However, we could potentially allow it to happen by tweaking the schema.

This PR goes through all blocks before writing them to parquet, and
merge schemas that differ only in nullability of the fields.
It also casts the table to the newly merged schema so that the write
could happen.

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number

Closes ray-project#48102

---------

Signed-off-by: rickyx <rickyx@anyscale.com>
…ay-project#48697)

## Why are these changes needed?

This makes SortAggregate more consistent by unifying the API on the
SortKey object, similar to how SortTaskSpec is implemented.


## Related issue number

This is related to ray-project#42776 and
ray-project#42142


Signed-off-by: Richard Liaw <rliaw@berkeley.edu>
@jjyao
Copy link
Collaborator

jjyao commented Nov 14, 2024

@JP-sDEV seems the PR has some rebase issue. It now contains lots of unrelated changes.

Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

(please rebase/cleanup)

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.

Fix flake8 rule E501: Line too long (82 > 79 characters)