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

Unpin setproctitle, fixes #37727 #40289

Merged
merged 1 commit into from
May 23, 2024
Merged

Unpin setproctitle, fixes #37727 #40289

merged 1 commit into from
May 23, 2024

Conversation

timkpaine
Copy link
Contributor

Why are these changes needed?

Related issue number

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 :(

@mattip
Copy link
Contributor

mattip commented Oct 13, 2023

@richardliaw this is needed for ray + python 3.11 on conda-forge. Could we get some eyes on it?

@richardliaw
Copy link
Contributor

could you merge master back in?

@mattip
Copy link
Contributor

mattip commented Oct 15, 2023

master was merged back in, there are still a number of failing CI jobs. Is that normal?

@mattip
Copy link
Contributor

mattip commented Oct 15, 2023

Once again this is out of date with master.

@mattip
Copy link
Contributor

mattip commented Oct 17, 2023

FWIW, the conda-forge 2.7.1 release incorporates this PR, which enables python3.11 packaging.

@mattip
Copy link
Contributor

mattip commented Oct 20, 2023

Merging with master is an ongoing process. Until this can get some review it probably is not worth the effort. FWIW, this is working well on conda-forge, where it has already been adopted. Is there a reason not to do this?

Copy link

stale bot commented Dec 15, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added stale The issue is stale. It will be closed within 7 days unless there are further conversation and removed stale The issue is stale. It will be closed within 7 days unless there are further conversation labels Dec 15, 2023
Copy link

stale bot commented Mar 17, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 17, 2024
@anyscalesam anyscalesam added the QS Quantsight triage label label Apr 22, 2024
@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Apr 22, 2024
@anyscalesam
Copy link
Contributor

@mattip is this specific to conda-forge only and not pip?

@mattip
Copy link
Contributor

mattip commented Apr 23, 2024

The version 1.2.2 in setup.py has wheels for python<3.10. pip install works currently by installing from source, which requires compiling the c-extension on each install. It would be much cleaner to merge this PR and use wheels.

@mattip
Copy link
Contributor

mattip commented Apr 23, 2024

I rebased the PR off master to make this mergable.

@timkpaine
Copy link
Contributor Author

Thanks @mattip!

@anyscalesam anyscalesam assigned aslonnie and can-anyscale and unassigned mattip May 14, 2024
@anyscalesam
Copy link
Contributor

this part of the stack sort of in no mans land; doesn't map directly to any eng team.... @aslonnie @can-anyscale thoughts on merging this?

@anyscalesam anyscalesam added triage Needs triage (eg: priority, bug/not-bug, and owning component) Devprod labels May 14, 2024
Copy link
Collaborator

@can-anyscale can-anyscale left a comment

Choose a reason for hiding this comment

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

accept to unblock, i think the change is safe;

if the code owners do not accept in the next few days I can help merge it

@timkpaine
Copy link
Contributor Author

rebased on latest master just to make sure its still ok

@aslonnie aslonnie removed their assignment May 23, 2024
@anyscalesam
Copy link
Contributor

@can-anyscale please just merge it

@can-anyscale can-anyscale enabled auto-merge (squash) May 23, 2024 17:15
@can-anyscale
Copy link
Collaborator

yes, merging, sorry for the delay

@github-actions github-actions bot added the go add ONLY when ready to merge, run all tests label May 23, 2024
@can-anyscale can-anyscale disabled auto-merge May 23, 2024 21:29
@can-anyscale can-anyscale merged commit 4fb670b into ray-project:master May 23, 2024
6 of 8 checks passed
can-anyscale added a commit that referenced this pull request May 24, 2024
@can-anyscale
Copy link
Collaborator

Could be breaking this test https://github.com/orgs/anyscale/projects/76/views/1?pane=issue&itemId=64249695 and is blocking the release. I'll revert if confirmed in #45539. Thankks.

@mattip
Copy link
Contributor

mattip commented May 25, 2024

I don't have access to that project page. What test is failing? Why isn't it failing in the CI here?

@can-anyscale
Copy link
Collaborator

@mattip ah yes sorry, it is this issue #45537

it's a macos test darwin://python/ray/tests:test_job so it doesn't run on CI

@timkpaine timkpaine deleted the tkp/spt branch May 26, 2024 14:00
@timkpaine
Copy link
Contributor Author

timkpaine commented May 26, 2024

I'll leave it to the anyscale team to ensure this eventually makes it in

@mattip
Copy link
Contributor

mattip commented May 30, 2024

The problem is here

assert submission_job["Entrypoint"] == list2cmdline(commands)

At this point list2cmdline(commands) is

'python /var/folders/4r/67pc2qss79q97y_7jpftm8tw0000gn/T/tmpei8y07tm --flag'

and job["Entrypoint"] is

'"/opt/homebrew/Cellar/[email protected]/3.11.2_1/Frameworks/Python.framework/Versions/3.11/Resources/Python.app/Contents/MacOS/Python \
/var/folders/4r/67pc2qss79q97y_7jpftm8tw0000gn/T/tmpei8y07tm --fla" "" LC_PAPER=en_AU.UTF-8'

The long name for python has something to do with me running the tests in a virtual environment, I have a fix to allow tests to run locally, but that is not the problem.
Note --flag is missing a g. This somehow has something to do with setproctitle: the older version does not cut off the g.

@mattip
Copy link
Contributor

mattip commented May 30, 2024

This is probable dvarrazzo/py-setproctitle#118, which is still open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Devprod go add ONLY when ready to merge, run all tests QS Quantsight triage label triage Needs triage (eg: priority, bug/not-bug, and owning component)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants