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

fixelcfestats: Catch track file usage (& discussion RE: git tags usage) #1834

Closed
Lestropie opened this issue Dec 5, 2019 · 22 comments
Closed

fixelcfestats: Catch track file usage (& discussion RE: git tags usage) #1834

Lestropie opened this issue Dec 5, 2019 · 22 comments
Assignees

Comments

@Lestropie
Copy link
Member

@Lestropie Lestropie commented Dec 5, 2019

If a user tries to run fixelcfestats by providing a track file rather than a fixel-fixel connectivity matrix, a more informative error message should be generated explaining the change and directing them to the online documentation.

@Lestropie Lestropie added this to the MRtrix3 3.0 release milestone Dec 5, 2019
@Lestropie Lestropie self-assigned this Dec 5, 2019
@boegel

This comment has been minimized.

Copy link
Contributor

@boegel boegel commented Jan 7, 2020

@Lestropie Is the documentation that explains the use of the connectivity matrix rather than a track file already documented? If so, where?

I'm helping someone out who's running into this exact problem using MRtrix 3.0_RC4, it seems:

fixelcfestats: [ERROR] required input "tracks_2_million_sift.tck" is not a directory
@Lestropie

This comment has been minimized.

Copy link
Member Author

@Lestropie Lestropie commented Jan 9, 2020

The online documentation is generated and made available for a number of different software versions, which can be navigated via the green text down in the bottom-left corner of the sidebar. The relevant contents are on the dev branch starting here.

3.0_RC4 isn't an announced release; I presume it's there for testing GitHub Actions / binary executable distributions / other "release" stuff. There currently isn't docs generated for that tag, probably because we weren't expecting anybody to be using it.

@civier

This comment has been minimized.

Copy link
Member

@civier civier commented Jan 9, 2020

@Lestropie

This comment has been minimized.

Copy link
Member Author

@Lestropie Lestropie commented Jan 9, 2020

The latest state of master and 3.0_RC4 are two very different software versions. 3.0_RC4 was tagged based on the state of dev at the time; as such, the current master is not a derivative of 3.0_RC4. Any up-to-date checkout of master should report its version as being derived from 3.0_RC3_latest, which is another tag being used for release testing.

We wouldn't expect users to directly check out the tags 3.0_RC3_latest or 3.0_RC4, since we've never alerted users to their existence.

Building the documentation is really fast. But I don't think there's any need to have an explicit docs build for 3.0_RC4 since, as I say, nobody should be checking that out directly. The documentation built from dev should be applicable to anyone whose software reports being 3.0_RC4 (since that's the most recent tag used to generate the version string for anyone who has checked out dev recently).

@boegel

This comment has been minimized.

Copy link
Contributor

@boegel boegel commented Jan 9, 2020

@Lestropie Thank you for clarifying. The fact that 3.0_RC3(_*) and 3.0_RC4 are two very different versions is quite confusing.

It's easy to assume that the 3.0_RC4 listed at https://github.com/MRtrix3/mrtrix3/releases just wasn't tagged as a release, using a sufficiently different tag would have made more sense I think.

So, with all that in mind, which tag/branch is considered to be the latest stable version, which people should be using right now? Is that 3.0_RC3_latest, or master (and if it's the latter, how different is master from 3.0_RC3_latest)?

@Lestropie

This comment has been minimized.

Copy link
Member Author

@Lestropie Lestropie commented Jan 9, 2020

Yes, more developer-oriented tag names may have been preferable. Wasn't me 🤷‍♂

Latest version of master is always the latest (& most) stable version, as it includes all latest hotfixes. Checking out master and building right now will report as "3.0_RC3_latest-68-gd411e6c5": the most recent tag is "3.0_RC3_latest", but there are 68 commits subsequent to that, which will all be bug fixes (we don't merge any non-egregious-bug behaviour-altering changes to master). If you were to check out tag 3.0_RC3_latest directly, you would be missing those 68 bugfix commits. If you were to check out 3.0_RC3 directly, you'd be missing the last 284 bug-fix commits on master.

After release we won't have this divergence between master and the most recent tag. master will point to the most recent release tag. Any hotfixes, instead of going to master without any corresponding tag, will instead be tagged according to semantic versioning and generate a new release.

@jdtournier

This comment has been minimized.

Copy link
Member

@jdtournier jdtournier commented Jan 9, 2020

Sorry, that one was my fault... I hadn't envisaged that people would check out these tags rather than master directly. I agree it would have been preferable to use a different tag name for this. I might remove that tag altogether if it creates problems. Most of the required testing for building of binaries and CI is done, there's no particular reason to hold on to the tag, especially as it seems to generate confusion...

But as @Lestropie says, this is going to be resolved fairly soon, hopefully. We're hoping to release a full-blown 3.0 in the coming weeks.

@boegel

This comment has been minimized.

Copy link
Contributor

@boegel boegel commented Jan 9, 2020

@jdtournier Please don't remove the tag, that's going to create even more confusion...

@civier

This comment has been minimized.

Copy link
Member

@civier civier commented Jan 9, 2020

Lestropie added a commit that referenced this issue Jan 9, 2020
Print a custom Exception if the user attempts to run fixelcfestats by providing a track file.
Closes #1834.
@boegel

This comment has been minimized.

Copy link
Contributor

@boegel boegel commented Jan 9, 2020

@civier Renaming the tag is just as bad as removing it, no need to make things more confusing than they are already? :)

@jdtournier

This comment has been minimized.

Copy link
Member

@jdtournier jdtournier commented Jan 9, 2020

@jdtournier Please don't remove the tag, that's going to create even more confusion...

Yes, I was wondering if that might be the case. I'm not sure what else to do here though, I clearly wasn't expecting users to install it like this. Naive of me, I guess... The simplest thing here would be a rename, as @civier suggests, but it's not simple since there needs to be a (very minor) code change to match. So I can't simply re-tag the existing version.

What I've done is to edit the release on GitHub with hopefully sufficient warnings... See what you think.

@jdtournier

This comment has been minimized.

Copy link
Member

@jdtournier jdtournier commented Jan 9, 2020

Just to answer a few more questions (and eat hefty amounts of humble pie):

I think it will be super-useful to users if this information wil be available on the main documentation (like Kenneth, I assumed that all tags are on master).

This is temporary glitch, I wasn't intending for this to happen. I'll make sure it doesn't happen again... If I need to tag versions not intended for release, I'll make sure they're not labelled with tags that look official.

Also, can you also clarify to me what was the purpose of 3.0_RC3_latest?

It's a similar issue, really. I wanted to distribute pre-compiled, but relatively up to date versions of the software for a workshop, and a release seemed like the right place to add the binaries. It's really just the latest version of master at that point. Again, I don't foresee needing this in future once 3.0 is out, and if I do, I'll make sure to do this on my own fork...

and why did you generate documentation for it? (if you did not announce it)

I'm not sure we did? The latest version of the docs is that generated from the tip of the master branch, and gets updated with every push to master. There's no 3.0_RC3_latest version on there as far as I can see?

Another thing that would be useful is to have an easy way to go from the release in github to the release announcement (or alternatively, to have the version announcement as part of the docs). Sorry if these things already exist, but even if they are, it is not obvious how to find them.

That's a good suggestion. I'll add a link now for those that have an announcement...


And just in case it wasn't clear from the above: please accept my sincere apologies for the confusion. This is clearly pretty amateurish on my part...

@civier

This comment has been minimized.

Copy link
Member

@civier civier commented Jan 9, 2020

@boegel

This comment has been minimized.

Copy link
Contributor

@boegel boegel commented Jan 9, 2020

What I've done is to edit the release on GitHub with hopefully sufficient warnings... See what you think.

That makes it pretty clear, without doing any damage (in terms of removing the tag), so 👍 imho, best possible remedy.

@jdtournier

This comment has been minimized.

Copy link
Member

@jdtournier jdtournier commented Jan 9, 2020

First of all, apology accepted (but you do owe me a favor, as my collaborator invested quite a lot of time in an analysis that we will probably have to repeat with a good version of MRtrix :-( ).

Yes, I can appreciate this isn't great.. We've only merged this tag 5 weeks ago though, hopefully they haven't invested too much effort yet...?

More to the point, there's probably no reason to repeat the analysis: there's nothing 'bad' about this version - no overt bugs that would impact the validity of the analysis (at least as far as know, depending on what the issue is in this post...). I'd probably hold fire on re-doing the entire thing until you've got a better idea of whether there's any need for that.

"latest" is really a confusing label, considering the "3.0_RC3_latest" label. I think it should be clarified somewhere that "latest" refer to the tip of master rather than the latest release. Actually, adding to "latest" the date of the master tip will be the best, if that possible.

This is the label given by readthedocs by default. We can probably change it to e.g. master, but that will probably break all kinds of links on the forum. And it has to remain static, since it's a permalink... So I vote it stays as it is.

But I do agree it's annoying that the docs themselves don't have a more explicit reference to the exact version they refer to, and the date they were generated. Maybe there's a way to do that with readthedocs' restructured text markup, we'd need to investigate.

@thijsdhollander

This comment has been minimized.

Copy link
Contributor

@thijsdhollander thijsdhollander commented Jan 9, 2020

Also, if you want to be on the safe side, let people know of the glitch in an announcement on the blog or a broadcast message on the forum. Maybe there are other groups in the same situation.

@civier yes, indeed. I know of a couple of places where cases of "RC4" popped up, but I've just managed them myself to avoid problems and lost time personally. I don't know what the intended strategy was/is, so I'm not touching the subject beyond that.

@civier

This comment has been minimized.

Copy link
Member

@civier civier commented Jan 10, 2020

@thijsdhollander

This comment has been minimized.

Copy link
Contributor

@thijsdhollander thijsdhollander commented Jan 10, 2020

Something that still strikes me is why github does not print out the branch of the tag?

No, that makes sense though: there's actually no concept of "the (unique) branch of the tag", so as to say. A tag points to a single commit, but a single commit can be "part" of many branches. The closest you can get to seeing the kind of information you're wondering about is this:

  • From the release/tags page, click on the commit that the tag points to. You can see this on the left, right below the name of the tag. In case of RC4, that takes you to this commit: b774c82

  • On that page you can see which branches "have" this commit somewhere in their history of commits. Depending on what kind of development or branching model developers use, this information can be more or less well structured or helpful.

@jdtournier

This comment has been minimized.

Copy link
Member

@jdtournier jdtournier commented Jan 10, 2020

OK, I've made the announcement - I hadn't realised this was already affecting that many people. Apologies once again.

Regarding the rationale behind this tag: there is none, it was a dumb mistake, and ridiculously thoughtless on my part. For the record, this tag started life on my own fork, where I was testing the use of GitHub Actions to automate the creation of binary installers for the various platforms we target. I needed a fresh tag to trigger the builds, and that's the one I used on my fork. Once it was all up and running, I created a pull request for it and it was merged, tag and all. It hadn't occurred to me by then that it would obviously look like the next 'official' version to any external user... I'll fully admit that was a pretty dumb thing to do. All I can do is apologise and endeavour not to screw up like this again.

@civier

This comment has been minimized.

Copy link
Member

@civier civier commented Jan 10, 2020

@jdtournier

This comment has been minimized.

Copy link
Member

@jdtournier jdtournier commented Jan 10, 2020

No, I completely agree that we should encourage users to stick to tagged versions - this is how it's supposed to be done. What is true is that because we've always recommended users stick to the master branch, I haven't so far given enough consideration to the obvious reality that users will (rightly) be going for tags - this might have contributed to this debacle. But it should absolutely be the case that official-looking tags should be considered stable. This is 100% my goof, and goes against our own de facto policy.

I'd personally really like to remove this tag given that it's causing more widespread confusion than I'd realised. But I understand the rationale for keeping it there now that it's been pushed out...

I suggest to add documentation that explains the logic behind the release cycles, with the hope that people will understand why it is beneficial for them to install the latest commit to master:
#1871

I agree we probably need to do this. The point of tagged releases is as you understand it - no debate there. Once we start releasing precompiled packages, this will be even more important (we can't generate packages for every single push to master - although we might consider nightly releases, but it's apparently not trivial on GitHub).

The reason we would advocate sticking to master (at least between tag changes) is that we are careful to only push overt bug fixes and non-behaviour altering changes to master. As long as the base tag remains the same (as reported by the -version option), then it's essentially the same code with annoying bugs fixed. Any new features or behaviour-altering changes will be pushed to the dev branch for the next release.

my reason for not installing the tip of master is the potential of being exposed to undocumented/undiscovered bugs due to recent bug fixes.

So we might slightly disagree on that, as per my above comment. I think you're better off sticking to master if you want to minimise exposure to bugs. But what is absolutely true is that we can't guarantee that behaviour will remain absolutely unchanged if your pipeline was subject to a bug that was fixed - we're careful to only push bug fixes to master if they're necessary and non-behaviour-altering, but there's no getting around the fact that it's different code...

One of the problems we face is that oddly enough, the tip of master is likely to be the most bug-free version of the corresponding version - it's had all the bug fixes, and only bug fixes. This is likely to be less of a problem once we start rolling out proper releases (which I'm hoping is imminent), since we'll be in a better position to release bug fix releases more frequently (with micro version increments, e.g. 3.0.1 -> 3.0.2, as per semantic versioning).

I would be happy therefore if the release cycle / install recommendations will be modified a bit, and I wonder if you'd be interested to open it for discussion?

Definitely. Happy to discuss what you have in mind on #1871. Even better if you want to draft something and create a pull request for it. 👍

This might also be a good candidate for the wiki section of the forum (which I haven't opened up publicly yet, but we will once we release), or the GitHub wiki itself, since it's likely to evolve somewhat over time and shouldn't be release-specific. Otherwise it's painful to make minor changes to the documentation given that each pull request has to pass through automated testing and a round of reviews... Note that it's not either/or: I think we should document the logic/policy for the versioning on the wiki, but also flag these up on the install instructions, with links to those sections of the wiki.

@Lestropie Lestropie changed the title fixelcfestats: Catch track file usage fixelcfestats: Catch track file usage (& discussion RE: git tags usage) Jan 11, 2020
@Lestropie

This comment has been minimized.

Copy link
Member Author

@Lestropie Lestropie commented Jan 11, 2020

Closed by #1869. Any subsequent discussion or changes RE: git tag usage on this project should be posted as a new Issue, linking to this thread.

@Lestropie Lestropie closed this Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.