Skip to content

Conversation

@prasad-sawantdesai
Copy link
Collaborator

renamed imaspy->imas
package name : imas-python
updated documentation
updated links which were pointing to git.iter.org
branch is tested on bamboo https://ci.iter.org/browse/IC-IG-19
pytest and documentation tested

Copy link
Collaborator

@maarten-ic maarten-ic left a comment

Choose a reason for hiding this comment

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

Hi Prasad,

That must have been quite some work! I've had a quick look at the changes, and overall it looks good. There are a couple of things that we should think about a bit better, summarized below:

  1. IMASPy had some logic and content related to the legacy imas interface. Mainly in the benchmarking, tests and training material. IMO this logic and content should be removed. For one, it is technically impossible to have both the legacy and imaspy interfaces loaded after this change (since both will use the imas package name). And second, it will confuse newcomers that don't have access to the legacy interface.

    I'd suggest to review all files that come up with the following command (on the current develop branch):

    imas-python$ git grep -P '(\W|^)imas(\W|$)'
  2. Should we actually rename the python module imaspy->imas? The API is not 100% compatible with the legacy imas interface (and will never be). If we rename imaspy->imas, the transition for existing codes will probably be more painful than when another module name is used:

    • Existing code will most likely break because the API is not compatible.
    • There is no way to have the legacy and the new interface available at the same time (since both need to be imported with import imas). If you have a workflow with multiple IMAS-ified python components, you need to upgrade everything at the same time, instead of allowing a gradual transition where components are updated one-by-one.

I'm happy to have a (short) discussion about this if needed. Note that I will be over at IO in two weeks, so we could do it then if there's no hurry :)

Cheers,
Maarten

Here we see the benchmark ``core_profiles.Generate.time_create_core_profiles`` was
repeated for multiple values of ``hli``: once for the ``imas`` HLI, and once for the
``imaspy`` HLI.
``imas`` HLI.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be reviewed in more detail. Having the ASV benchmarks compare against the legacy imas interface doesn't make sense when renaming this package to imas.

Copy link
Collaborator Author

@prasad-sawantdesai prasad-sawantdesai Jan 23, 2025

Choose a reason for hiding this comment

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

Updated this file https://imas-python.readthedocs.io/en/latest/benchmarking.html, Could you please check it

@olivhoenen
Copy link
Collaborator

Hi Prasad,

That must have been quite some work! I've had a quick look at the changes, and overall it looks good. There are a couple of things that we should think about a bit better, summarized below:

  1. IMASPy had some logic and content related to the legacy imas interface. Mainly in the benchmarking, tests and training material. IMO this logic and content should be removed. For one, it is technically impossible to have both the legacy and imaspy interfaces loaded after this change (since both will use the imas package name). And second, it will confuse newcomers that don't have access to the legacy interface.
    I'd suggest to review all files that come up with the following command (on the current develop branch):
    imas-python$ git grep -P '(\W|^)imas(\W|$)'

Agree on the cleaning/removing of all AL legacy mentions.

  1. Should we actually rename the python module imaspy->imas? The API is not 100% compatible with the legacy imas interface (and will never be). If we rename imaspy->imas, the transition for existing codes will probably be more painful than when another module name is used:

    • Existing code will most likely break because the API is not compatible.
    • There is no way to have the legacy and the new interface available at the same time (since both need to be imported with import imas). If you have a workflow with multiple IMAS-ified python components, you need to upgrade everything at the same time, instead of allowing a gradual transition where components are updated one-by-one.

In terms of work needed in applications, keeping imaspy, moving to imas or inventing a third option won't change what needs to be done, except in the import lines. So let's use the package name that makes more sense, which is imas, and let's make sure we indicate the needed API change with the correct major release version.

I'm happy to have a (short) discussion about this if needed. Note that I will be over at IO in two weeks, so we could do it then if there's no hurry :)

Cheers, Maarten

Copy link
Collaborator

@olivhoenen olivhoenen left a comment

Choose a reason for hiding this comment

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

LGTM

@olivhoenen olivhoenen linked an issue Jan 21, 2025 that may be closed by this pull request
@olivhoenen olivhoenen force-pushed the rename-imaspy-to-imas branch from b2a398a to d7c6c5d Compare January 21, 2025 13:43
@Nush395 Nush395 mentioned this pull request Jan 21, 2025
@maarten-ic
Copy link
Collaborator

maarten-ic commented Jan 21, 2025

Just did a quick review of the documentation on https://imas-python.readthedocs.io/en/latest/, a couple of things that should be updated to improve the experience for newcomers:

  1. Installation instructions don't work: https://imas-python.readthedocs.io/en/latest/installing.html#development-installation

    • git clone ssh://git@github.com:iterorganization/imas-python.git
      Cloning into 'imas-python'...
      ssh: Could not resolve hostname github.com:iterorganization: Name or service not known
      fatal: Could not read from remote repository.
      
    • cd imas
      

      git by default creates a folder imas-python when cloning the project

    • Testing the install: both pytest commands fail

      edit: some of this may be fixed in Modifications for compatibility with TORAX. #11 already, please check before working on this.

  2. 5 minute introduction has many examples which cannot be run if you don't have imas_core available. I'd suggest to add a paragraph clarifying that imas_core is not yet open-sourced (and perhaps a timeline for when it will be)?

  3. Should you detail the CI config on ITER Bamboo in the public documentation? https://imas-python.readthedocs.io/en/latest/ci_config.html

  4. Benchmarking page should remove mentions of the "traditional HLI" and update the output. Parametrizing the HLI is not relevant anymore, since there is no possibility to benchmark against the old HLI: https://imas-python.readthedocs.io/en/latest/ci_config.html

N.B. the diff of this PR has now become so big that I'm not going to review it in further detail.

Copy link
Collaborator

@SimonPinches SimonPinches left a comment

Choose a reason for hiding this comment

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

As discussed this morning...

@olivhoenen
Copy link
Collaborator

Replaced by #14

@olivhoenen olivhoenen closed this Jan 23, 2025
@prasad-sawantdesai prasad-sawantdesai deleted the rename-imaspy-to-imas branch February 3, 2025 12:33
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.

Improve dd_helpers.py to avoid relying on Saxon JAR Update PyPi

5 participants