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

should not fail when no version in python_cluster_paths #41

Conversation

faucct
Copy link
Collaborator

@faucct faucct commented Oct 9, 2024

No description provided.

@faucct faucct requested a review from alextokarew October 9, 2024 08:33
@faucct faucct force-pushed the feature/should-not-fail-when-no-version-in-python_cluster_paths branch from 15523f8 to 963939a Compare October 9, 2024 08:49
Copy link
Collaborator

@alextokarew alextokarew left a comment

Choose a reason for hiding this comment

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

How do you ensure that you have an appropriate Python version on the cluster and which one to use for running cluster parts of the job?

@faucct
Copy link
Collaborator Author

faucct commented Oct 10, 2024

How do you ensure that you have an appropriate Python version on the cluster

I assume that the cluster has the matching version and the UDFs will fail to start if it doesn't.

which one to use for running cluster parts of the job?

I am not sure if I understood the question, but my code uses different python executables based on the supplied version.

@alextokarew
Copy link
Collaborator

I've checked it on out cluster an it seem to work, so the idea to get rid of `` sounds reasonable. But you also need to do modifications in these parts of the project:

Write a compat comment for not os_release block that it is preserved for backward compatibility and is subject to remove
https://github.com/ytsaurus/ytsaurus-spyt/blob/main/tools/release/publisher/config_generator.py#L147

The else clause can be safely removed, but check your clusters, maybe they also using old spyt version, in this case it also should be preserved and marked with comment
https://github.com/ytsaurus/ytsaurus-spyt/blob/main/tools/release/publisher/config_generator.py#L159

Remove all python_cluster_paths related code in YTsaurusOperationManager starting from here:
https://github.com/ytsaurus/ytsaurus-spyt/blob/main/resource-manager/src/main/scala/org/apache/spark/scheduler/cluster/ytsaurus/YTsaurusOperationManager.scala#L340

The tests should also be modified:
https://github.com/ytsaurus/ytsaurus-spyt/blob/main/e2e-test/common/cypress.py#L31

And this legacy atavism should also be removed
https://github.com/ytsaurus/ytsaurus-spyt/blob/main/yt-publish-plugin/src/main/scala/YsonableConfig.scala#L53

@faucct faucct requested a review from alextokarew October 22, 2024 09:03
@faucct faucct force-pushed the feature/should-not-fail-when-no-version-in-python_cluster_paths branch from 4b7a534 to e4b2957 Compare October 22, 2024 09:08
Copy link

robot-magpie bot commented Oct 24, 2024

@alextokarew has imported your pull request. If you are a Yandex employee, you can view this diff.

@alextokarew
Copy link
Collaborator

This PR #44 was successfully merged so we can close this one

@faucct faucct closed this Oct 24, 2024
@faucct faucct reopened this Oct 24, 2024
@faucct
Copy link
Collaborator Author

faucct commented Oct 24, 2024

It is another one that should be closed.

@faucct faucct requested a review from alextokarew October 24, 2024 19:40
Copy link

robot-magpie bot commented Oct 25, 2024

✅ This pull request is being closed because it has been successfully merged into our internal monorepository.
Your changes will be pushed to this repository soon. Thank you for your contribution!

@robot-magpie robot-magpie bot closed this Oct 25, 2024
robot-piglet pushed a commit that referenced this pull request Oct 25, 2024
No description

---

Pull Request resolved: #41
commit_hash:89146f50d1632032aea98ce23463b3babfe3df66
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