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

Chapel update for Arkouda #1

Closed
wants to merge 9 commits into from
Closed

Chapel update for Arkouda #1

wants to merge 9 commits into from

Conversation

bonachea
Copy link
Collaborator

No description provided.

Copy link
Collaborator Author

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

I created this staging PR from @arezaii 's code in order to annotate with comments.

I've made a few suggestions for improvement that I'd like to see considered or addressed before this is proposed/merged upstream.

var/spack/repos/builtin/packages/chapel/package.py Outdated Show resolved Hide resolved
var/spack/repos/builtin/packages/chapel/package.py Outdated Show resolved Hide resolved
var/spack/repos/builtin/packages/chapel/package.py Outdated Show resolved Hide resolved
@arezaii
Copy link
Owner

arezaii commented Nov 20, 2024

I think I addressed all the comments. There might be one more change coming to patch the older versions of chapel due to chapel-lang/chapel#26261, but should just be a patch

@bonachea
Copy link
Collaborator Author

This looks much better (based on source inspection).

Thanks for the improvements!

@arezaii
Copy link
Owner

arezaii commented Nov 21, 2024

I'm curious about your thoughts w.r.t adding this to the upcoming 2.3 release PR vs. making it a separate PR vs. possibly combining it with the arkouda package PR. I think putting it in the 2.3 release PR is reasonable but is likely to delay the arkouda package PR approval since it depends on these changes. I don't think that putting changes to multiple packages in one PR is the spack recommendation, so I'm leaning towards making it a separate PR for expediency.

@bonachea
Copy link
Collaborator Author

I don't think that putting changes to multiple packages in one PR is the spack recommendation, so I'm leaning towards making it a separate PR for expediency.

I agree that keeping this separate from the PR adding Arkouda seems like a good idea, unless we're really in a hurry to get the Arkouda spackage merged (in which case a combined PR might be justified).

adding this to the upcoming 2.3 release PR vs. making it a separate PR vs. possibly combining it with the arkouda package PR. I think putting it in the 2.3 release PR is reasonable but is likely to delay the arkouda package PR approval since it depends on these changes.

I'd probably recommend creating a PR for these Chapel changes soon (as soon as you're confident they are final), and hopefully that gets approved quickly. If for some reason that drags on, you can always add the 2.2 release to the same PR when that becomes available.

Copy link
Collaborator Author

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

New changes look great on source inspection (I have not tested, but assume you have).
Thanks for the improvements!

I made one minor suggestion for improvement.

Copy link
Collaborator Author

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

Thanks for all the improvements!

@arezaii
Copy link
Owner

arezaii commented Dec 11, 2024

I went ahead and moved this branch to chapel-2.3 to make the release PR spack#48053

@arezaii arezaii closed this Dec 11, 2024
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