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 [email protected] package to registry via mason publish" #74

Merged
merged 5 commits into from
May 23, 2024

Conversation

lucaferranti
Copy link
Contributor

new release of ForwardModeAD to support chapel 2.0

@lucaferranti
Copy link
Contributor Author

Before merging, please see my question in #62 . Do I need to manually create the tag release even if I ran mason publish?

@benharsh
Copy link
Member

benharsh commented May 7, 2024

mason publish is supposed to add the tag, but there appears to be a bug here. I'll be opening an issue shortly to capture this problem, and will update this comment with a link.

In the meantime, I think the best thing would be to just create the tag manually.

We are also looking in to the cause of these CI failures and will get back to you when we know more!

@lucaferranti
Copy link
Contributor Author

lucaferranti commented May 14, 2024

ok it seems we've unlocked the next level of of the mason publish quest

Main Module Check:
   Packages with more than one modules cannot be published. (FAILED)
------------------------------------------------------
Checking for fields in manifest file:
   Missing fields in manifest file (Mason.toml). (FAILED)
   The missing fields are as follows: 
   source

I guess the second one is just the Mason.toml having changed format for 2.0 and me failing to properly check the docs. I'll look at it.

The first one is a bit more interesting. I think of my library as a a single module, but I also want to structure that in multiple files for better readability/modularity. The main file (in this case ForwardModeAD.chpl) is just

module ForwardModeAD {

  public use Math;

  public use dualtype;
  
  public use arithmetic;

  public use trigonometric;

  public use transcendental;

  public use hyperbolic;

  public use differentiation;
}

The dualtype defines the dual number types. The files from arithmetic to hyperbolic overload functions for dual numbers and the last file has the logic to compute derivatives.

The main file only imports and re-exports all "sub-modules". It seems Mason does not like it anymore after 2.0? Why is that?

(feedback on how to better structure my library is of course also welcome, if my setup has some drawbacks. I do find having a lot of code in a single file a bit unergonomic though and prefer to split in multiple files. Especially for bigger libraries (although mine arguably is not there yet) being able to structure it in a modular way sounds pretty important to me).

@lucaferranti
Copy link
Contributor Author

For the second one:

I checked the documentation for Manifest file (https://chapel-lang.org/docs/main/tools/mason/guide/manifestfile.html) and as far as I can tell, the source field seems to belong to the Mason.lock file but not to Mason.toml.

The toml file commited here (0.2.0.toml) does have the source field.

What is the idea of that check?

@lucaferranti
Copy link
Contributor Author

lucaferranti commented May 16, 2024

Fixed the missing source field issue 🎉 (and opened an issue about updating the documentation for it)

What about the other problem

(FAILED) Your package has more than one main module

?

I see the same issue in #73 , so it seems a common pattern (technically 2/3 of the packages trying to support Chapel 2.0 have this problem :P ), what is the motivation behind that restriction? Can it be lifted?

@lucaferranti
Copy link
Contributor Author

lucaferranti commented May 16, 2024

ok, I think I've found something. My structure is flawn, as I should use a ForwardModeAD folder for the submodules. Nevertheless the package in #73 has a proper structure and the check still fails. I believe this is a bug in checkModule logic and I've opened an issue to discuss about it

@lucaferranti
Copy link
Contributor Author

I see someone restarted the workflow of this a few minutes ago. I need to still fix the structure of my package, doing it now :)

Besides, wouldn't I need to wait for the next release anyway? Or does the registry use Chapel nightly build?

@benharsh
Copy link
Member

That's my bad, I forgot that the package still needed updating. No need to rush and fix it right now!

If I'm reading things correctly then the CI job should be pulling from main, and therefore should be able to incorporate the "more than one module" fix from yesterday.

@lucaferranti
Copy link
Contributor Author

If I'm reading things correctly then the CI job should be pulling from main, and therefore should be able to incorporate the "more than one module" fix from yesterday.

I see, interesting idea, why not just pulling the latest stable docker image? Is it because the registry is still "maturing" and it's more valuable to have latest main with potential fixes (like in this case)?

@benharsh
Copy link
Member

Is it because the registry is still "maturing" and it's more valuable to have latest main with potential fixes (like in this case)?

I believe that's mainly it, yes.

@lucaferranti
Copy link
Contributor Author

lucaferranti commented May 22, 2024

Ah, now I remember why I hadn't update the structure yet, I needed to ask something about modules includes and visibiliy, but I'll ask that on discourse, since it's probably something useful to other chapel library developers too

edit: funnily enough I had asked this question 1 year ago (for curiosity, before having the issue) https://chapel.discourse.group/t/importing-files-from-subfolders/17064, that's what I call thinking ahead of time xD

@lucaferranti
Copy link
Contributor Author

posted a follow-up comment on discourse for better visibility: https://chapel.discourse.group/t/importing-files-from-subfolders/17064/8

here is the PR shuffling files around if someone wants to have a look: lucaferranti/ForwardModeAD#16 feedback on what is an idiomatic structure welcome (except for your library is small, just put everything in one file :P)

The PR should in the end be fairly simple to navigate:

  • DualType.chpl defines basic types needed in the other files
  • ElementaryFunctions.chpl and Differentiation.chpl use DualType but are independent from each other

should hence be enough to look at the first 3 lines of each file, the rest is just some boring maths :)

@lucaferranti
Copy link
Contributor Author

lucaferranti commented May 23, 2024

@benharsh CI green and sunny 🌞

@benharsh benharsh merged commit dcc9e0e into chapel-lang:master May 23, 2024
1 check passed
@benharsh
Copy link
Member

Thanks for sticking with it while we worked through these CI issues!

@lucaferranti lucaferranti deleted the ForwardModeAD branch May 23, 2024 16:23
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