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

require source installation in metadata #98

Closed
ianmkenney opened this issue Nov 13, 2023 · 13 comments
Closed

require source installation in metadata #98

ianmkenney opened this issue Nov 13, 2023 · 13 comments
Labels
policy rules and requirements (optional/mandatory) for a MDAKit

Comments

@ianmkenney
Copy link
Member

Since the FAIR4RS requires that source is accessible, we should make the src_install field required. From there I think it's fair to keep install as an optional field.

@orbeckst orbeckst added the policy rules and requirements (optional/mandatory) for a MDAKit label Nov 13, 2023
@IAlibay
Copy link
Member

IAlibay commented Nov 13, 2023

So there's a difference between src_install and project_home, the latter is the one that we want to enforce for FAIR4RS.

The reason for this distinction, and not enforcing a src_install, is because early on in the MDAKits process we agreed that we wanted to be able to handle mdakits being submitted that weren't fully packaged - i.e. that sometimes someone might come along with a script in a github repo and that would be ok.

That being said, we should revisit this discussion.

@orbeckst
Copy link
Member

Thanks for the clarification. I didn't know the intended semantic difference between src_install and project_home.

However, similar to discussion in #95 (comment) : They have to tell us how to install their package or we cannot do our job. We do require installation instructions so they must be able to provide them in a form that we can execute.

Therefore, I am also in favor to make src_install mandatory.

I recognize that install is phrased ambiguously

a list of commands to use when installing the latest release of the code

and one could argue that install should be mandatory with whatever instructions are needed, possibly duplicating what's in src_install. However, I don't support making install mandatory because effectively we have already taken this to mean "install as a package". If it keeps this meaning (and I propose that we keep it and re-inforce it because that's what most users would consider "installing") then install should remain optional. Furthermore, if they provide source installation instructions in install then it's equally easy to provide them in src_install.

@orbeckst
Copy link
Member

Btw, what happens in the registry if either install or src_install (or both) are missing, @IAlibay ?

@IAlibay
Copy link
Member

IAlibay commented Nov 13, 2023

Btw, what happens in the registry if either install or src_install (or both) are missing, @IAlibay ?

I believe the badges just go "grey" - i.e. "could not run this".

@IAlibay
Copy link
Member

IAlibay commented Nov 13, 2023

We do require installation instructions so they must be able to provide them in a form that we can execute.

That is a very good point. I'm also convinced, however I think the "looser" requirements came from other folk's views (I think @jbarnoud might have been a proponent of this?). Either way, we should at least rope in @lilyminium and @fiona-naughton on this discussion.

@jbarnoud
Copy link

jbarnoud commented Nov 14, 2023

I haven't followed mdakit for a long while so I am lacking context. Bouncing on an earlier comment of Irfan, the two main ideas I had about the project were:

  • the amount of mandatory maintenance should be minimal (it can keep working if nobody has time to regularly spend on it)
  • the barrier for entry should be as low as possible to encourage new comers in the mdakit ecosystem. Often, we have PhD candidates or postdoc that have tools but won't share their code because they do not know how. Rather than scare them by telling them their project is not good enough, we should encourage them to share first and then give them incentives to improve. For instance: create a package and we can add you to our ci and add to to the better list of kits.

@orbeckst
Copy link
Member

I agree that we want to encourage people to share. Minimal testing is, however, one of those requirements where we drew a line, as spelled out in our requirements for becoming a registered kit https://mdakits.mdanalysis.org/about.html#what-is-an-mdakit . I don't think we want to remove this requirement, do we?

Personally, I do believe in the "untested code is broken code" adage" and I see part of our mission to educate on minimal best practices. From my experience, people are more motivated to go the extra mile when they want to achieve something (like getting a kit registered) as opposed to "coming back later to add tests when I have time".

If we want a two-tiered registry where we have tested kits with green badges and a "second tier" of kits that are effectively just links without badges or red badges then we need to discuss it.

and we can add you to our ci

That's exactly what we're doing, but in order to do it we need to know

  • How to install your package?
  • How to run your tests?

@orbeckst
Copy link
Member

@fiona-naughton and @lilyminium please have a look at this issue and the related #95 on making three important parameters in the registration yaml file required. Your comments would be very much appreciated!

The parameters are

  • source_install: commmands to install the software from source

    ## List(str): a list of commands to use when installing the mdakit from its
    ## source code.
    src_install:
    - pip install git+https://github.com/GH_HOST_ACCOUNT/MYPROJECT@main

  • run_tests and test_dependencies: commands to run tests and to install any deps for the tests

    ## List(str): a list of commands to use when attempting to run the MDAKit's tests
    ## If you package your tests inside your package then you can typically use the
    ## pytest --pyargs MYPACKAGE
    ## command as shown below.
    ## Otherwise you need to include commands to make the tests available.
    ## For example, if the tests are in the repository at the top level under `./tests`:
    ## First use `git clone latest` to either clone the top commit for "development code" checks or check out
    ## the latest tag for "latest release" checks. Then then run pytest:
    ## - git clone latest
    ## - pytest -v ./tests
    ## Feel free to ask for advice on your pull request!
    run_tests:
    - pytest --pyargs MYPACKAGE
    ## List(str): a list of commands to use to install the necessary dependencies required
    ## to run the MDAKit's tests.
    ## The default below _might_ be sufficient or you might not even need MDAnalysisTests:
    ## make sure that it is appropriate for how you run tests.
    test_dependencies:
    - mamba install pytest MDAnalysisTests

Requirements for a (registered) MDAKit are in the paper and summarized on our web site.

One reason why they are currently optional was related to keeping the barrier to entry low, as expressed in #98 (comment) and #98 (comment) .

@fiona-naughton
Copy link
Contributor

Restarting this discussion having been reminded when writing the reviewer's guide - my apologies for not having commented at the time.

I would tend to agree with Oliver; I understand stuff like writing tests can be a daunting barrier, but I would consider it one of the key aims of MDAKits to be making this barrier as low as possible so we are building up scientists who know how to write better code (with the cookie cutter and instructions, hopefully some additional resources I'm planning to put together, reviewers there to provide advice, the incentive of registering a kit, etc), rather than removing it entirely by making it optional - which just delays the issue (similar to what Oliver said, if that first step isn't now, I'm not sure I see them coming back to take it later). It's also in line with how we've been selling it to users.

(It doesn't need to be super comprehensive tests - the biggest barrier imo is understanding the general idea and format, so as long as they've encountered that, hopefully building out more coverage is a lot less scary and more likely to happen)

(Also, it'll be easier to relax a compulsory rule in the future if we do decide it's too off putting, then have to instate it and need to sort through everything that's already been registered)

@orbeckst
Copy link
Member

I'd like to revive this issue together with #95 and come to a decision. Can we discuss at the next MDAKits meeting?

@orbeckst
Copy link
Member

I put it on the agenda for the 2024-07-17 meeting.

@orbeckst
Copy link
Member

At the EOSS4 MDAKits meeting on Jul 17, the MDAKits team decided that we want to make minimal tests and installation from source MANDATORY and thus enforce it during reviews.

In a subsequent round of consultations with the CoreDevs, there were a number of positive comments and no objections.

Therefore, minimal tests and installation from source instructions are MANDATORY and enforceable during review.

@orbeckst
Copy link
Member

Completed with PR #159

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
policy rules and requirements (optional/mandatory) for a MDAKit
Projects
None yet
Development

No branches or pull requests

5 participants