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

Add ecosystem.json to include project in Qiskit ecosystem #43

Closed
wants to merge 1 commit into from

Conversation

ElePT
Copy link
Collaborator

@ElePT ElePT commented Aug 16, 2023

Summary

Adds an ecosystem.json file to pass necessary checks to add project to Qiskit ecosystem.

Details and comments

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5876576652

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.947%

Totals Coverage Status
Change from base Build 5868912123: 0.0%
Covered Lines: 6451
Relevant Lines: 7172

💛 - Coveralls

Copy link
Collaborator

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

I would expect we need to make some changes from the example file, as we use different tools and already have some workflows in place 🙂 But I'm also not sure if this is correct -- however opening an issue and letting the ecosystem's CI run would probably resolve it 😄

"versions": ["3.9"]
},
"tests_command": [
"pytest"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we just defer this to the Makefile we have already? So

Suggested change
"pytest"
"make test_ci"

Copy link

@mickahell mickahell Aug 18, 2023

Choose a reason for hiding this comment

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

Just keep in mind in Ecosystem we launch it inside a tox env, so keep it as simple and basic as possible :)

Choose a reason for hiding this comment

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

You'll need to add :

    "debian_dependencies": [
        "make"
    ],

The feature is actually in review process

"pytest"
],
"styles_check_command": [
"pylint -rn src tests"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Likewise this could maybe be

Suggested change
"pylint -rn src tests"
"make lint style"

Choose a reason for hiding this comment

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

Just keep in mind in Ecosystem we launch it inside a tox env, so keep it as simple and basic as possible :)

Choose a reason for hiding this comment

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

You'll need to add :

    "debian_dependencies": [
        "make"
    ],

The feature is actually in review process

"requirements-dev.txt"
],
"extra_dependencies": [
"pytest"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the test dependencies are in requirements-dev.txt which is in the dependencies above, maybe we don't need this?

Choose a reason for hiding this comment

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

Just check it, what do you mean ?
I even don't see pytest in your 2 files

@woodsp-ibm
Copy link
Member

I would have to ask whether this is really needed - it was not done for any of the apps. I guess my concern is over running duplicate? CI over the tests? I get that if you are submitting a new project having some CI run is good so ecosystem provisioing something is helpful - where is this run under the ecosystem's repo github actions. But in this case we have a comprehensive CI that runs already here - and flags in the readme, via the ribbon, whether tests are passing etc which I guess ends up as as outcome of whatever ecosystem runs. Would it not be better to just link this in someway assuming this is doable in the ecosystem. If it was setup for this it could hooked up for the apps too so their status was visible in the ecosystem project cards.

@mickahell
Copy link

I would have to ask whether this is really needed - it was not done for any of the apps. I guess my concern is over running duplicate? CI over the tests? I get that if you are submitting a new project having some CI run is good so ecosystem provisioing something is helpful - where is this run under the ecosystem's repo github actions. But in this case we have a comprehensive CI that runs already here - and flags in the readme, via the ribbon, whether tests are passing etc which I guess ends up as as outcome of whatever ecosystem runs. Would it not be better to just link this in someway assuming this is doable in the ecosystem. If it was setup for this it could hooked up for the apps too so their status was visible in the ecosystem project cards.

Hello @woodsp-ibm :)

I'm Michael one of the creator of the Ecosystem and I'm trying to continue contributing to.
The file is mainly necessary to take into account your requirements-dev.txt, by default we only take the classical one. The state of the ecosystem.json file @ElePT pushed is mainly the template one and the one I'm suggesting to be as clause as possible to any project who need one (very basic template repo).

If you want to test before merging you can set the branch of this PR as default and submitting it in an Ecosystem fork (I can do it for you if you want)

@woodsp-ibm
Copy link
Member

@mickahell If we need to have a json file its fine by me. My comment was more around the fact that the repo heres has its own CI and does not need anything run elsewhere - and most likely what is here is way more comprehensive. Couple this with the fact that any number of other repos are part of Ecosystem (qiskit-aer, runtime, provider, experiments, nature, etc) that do not have an ecosystem.json, so however these were moved from qiskit org and became ecosystem projects I am not quite sure why this is being treated different. Not that I mind that it is as such, more I was questioning things since the changes noted above all seem to relate to CI being run for this - in the ecosystem repo I think if I understand how things are. How that's triggered and when it runs I have no idea. And since we have CI here with info about tests passing etc I guess I was wondering if that's all that was wanted to be shown on the ecosystem card (though I note none of the other repo's above have that either) is there no way to leverage this whereby we could include some link or other in the json for status rather than having to use resources to runs tests just to do that when the whole CI is already run here.

@mickahell
Copy link

hmm ok, I think it's just a misunderstanding.

Project in Ecosystem are just to present on the page https://qiskit.org/ecosystem for the public. There it has project from IBM, community and partner. Adding a project there doesn't mean your project goes to community project at all. The default tier is community but we'll allow to people to set another tier in the next release.

The process to check : the Ecosystem gonna scan your project if you don't have ecosystem.json he gonna execute its defaults commands and generate a tox file (same if you don't have setup.py). By default we only install the basic requirements.txt that why you need an ecosystem.json because for your tests you need to install your requirements-dev.txt.
This process is not for you to check if your project work, it's for people to see with which version of Qiskit your project can works. And also it's a minimum wage of quality.

With every project having their own CI and diff way to do so, we set ours own and we created the ecosystem.json to allow people to translate their CI into ours. At the beginning the goal was to only accept projects who pass our quality checks, that's not true today as we help a lot of people to make their project pass and some checks have been disabled.

And about which qiskit project is treat and how, I don't know. I think the main qiskit project are adding by hand with test off, but I'm not sure. For that maybe @frankharkins could help us.

I hope, what I wrote is clear enough and I answer your though @woodsp-ibm :)

@frankharkins
Copy link

Sorry I missed this. We'll be dropping the testing feature of the Ecosystem soon, so this PR is no longer needed.

@woodsp-ibm
Copy link
Member

@frankharkins Thanks - but what we we really want/need the qiskit-algorithms listed in the ecosystem, that was the real goal behind this PR. qiskit-nature etc were all added somehow so they are listed there (like qiskit-experiments, qiskit-aer etc none of these have this file), but qiskit-algorithms does not show up. Maybe someone manually added the apps etc. so that this is yet to be done for this project.

@frankharkins
Copy link

Sorry, I should have clarified; I've added the project to the Ecosystem in Qiskit/ecosystem#485, and it should appear on https://qiskit.org/ecosystem when that's next rebuilt (I believe nightly).

Let me know if you need anything else.

@woodsp-ibm
Copy link
Member

...I've added the project to the Ecosystem...

Perfect, thanks.

@woodsp-ibm
Copy link
Member

qiskit-algorithms is now listed as a project in Qiskit Ecosystem so I am closing this PR as it's no longer needed as noted above.

@woodsp-ibm woodsp-ibm closed this Oct 11, 2023
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.

6 participants