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

Time for a new Batchspawner release #103

Closed
mbmilligan opened this issue Jul 30, 2018 · 33 comments
Closed

Time for a new Batchspawner release #103

mbmilligan opened this issue Jul 30, 2018 · 33 comments
Milestone

Comments

@mbmilligan
Copy link
Member

I think we have enough changes queued up to merit a new release in the near future.

Consider this space a placeholder - we will fill it in with a list of issues we want to resolve/PRs we want to pull for this release.

I will go through the list of these items and choose some, but in the meantime if folks want to nominate items for the release please leave them in the comments on this issue. Thanks!

@rkdarst @cmd-ntrf @willingc

@rkdarst
Copy link
Contributor

rkdarst commented Jul 31, 2018

My thoughts:

Pull (of these my integration branch has #99, #97, #96, #94, #106/#107 and it seems to work):

To do:

we can possibly close:

@mbmilligan
Copy link
Member Author

Thanks for your work going through all those! With the caveat that we can always change our mind if some item becomes problematic, I'm okay using this as a starting point.

@rkdarst
Copy link
Contributor

rkdarst commented Aug 7, 2018

#112 has been created which integrates everything needed for the release. Just like the last release, waiting for PRs one at a time seemed to not be very feasible.

I'd say mainly we are waiting for confirmation on #109 and then any other changes from maintainers.

Please comment on there, and anyone can feel free to make pushes or PRs to that branch to support the release.

@mbmilligan
Copy link
Member Author

Ok, now that the big integration branch is merged, we're almost there. Thanks @rkdarst for doing most of the actual work!

Is it correct that #109 did not go in with the dev branch merge? If not, it looks like it's ready to go in.

The remaining issues appear to all relate to port number setting. Where do we stand on #102 vs #114 - and have they been tested to not conflict with #58 ?

@rkdarst
Copy link
Contributor

rkdarst commented Aug 11, 2018

Correct, #109 wasn't there. But it reports green now so should be good to go.

For the port number things... #114 fails on JH 0.7.2, I looked and superficially it seems like it should still work, but I guess somewhere else it doesn't use the returned port. Either someone needs to dig deeper, or we can skip the test on 0.7.2.

And it will certainly require some merging with #58, both code level and JH level. For example, currently info on port selection isn't passed to the singleuser server at all, so it's (port_range function) or (remote port selection) but not both. Someone with deeper knowledge than me should evaluate #58 and ensure everything will go smoothly, in particular there are no weird problems with asyncness or whatever. Maybe I can get to this sometime, but I'm not the best one here.

@cmd-ntrf
Copy link
Contributor

Regarding #58, I have updated the PR to allow port selection, fixed the tests and have the code up to date with the master branch. I have also put some explanations on why some tests fail in the PR comments.

@rkdarst Let me know if I can anything to ease the merge of PR #58.

@guillaumeeb
Copy link
Contributor

High everyone, is the release planned anytime soon? Let me know if something is needed in #109.

I'd like to have a clean release to make an "official" installation of Jupyterhub in my organization.

@rkdarst
Copy link
Contributor

rkdarst commented Oct 1, 2018

I'd like to do the release, but I can't myself. It would be good if someone could comment on other PRs.

I occasionally make my own integration branches when stuff is too slow, but that's more of a workaround when I need to deploy. I imagine I will get to upgrading our batchspawner within a few weeks, so I'll probably make a big push then.

Is there any way that the community can help with batchspawner more?

@guillaumeeb
Copy link
Contributor

Will be happy to help more too.

@mbmilligan
Copy link
Member Author

Merged #109. Will dig into #58 and #114 in the next day or two and see if we can come to a good resolution.

@mbmilligan
Copy link
Member Author

Merged @willingc tweaked version of PR #115 (which became #124 ) and now tests are working again, hooray. #58 appears to be in a good state now, so I've merged that too. It is available in the main branch so we can decide if #114 will need to work around it.

@rkdarst and others, are there other PRs that need merging ahead of cutting a new release?

@mbmilligan
Copy link
Member Author

Just a note: now that we are getting a little bit of testing on the current state of master, I think we need to sort out issues #126 and #127 before making a release. The changes do need documentation, but also I think users shouldn't have to add an import to their config file unless they are actually using the new feature.

@cmd-ntrf
Copy link
Contributor

I agree, having users import batchspawner.api was never part of the plan. All of my tests were with batchspawner directly, which explain why I missed potential issue with ProfileSpawner.

@guillaumeeb
Copy link
Contributor

Hi everyone, it's been a year, and still no new release of Batchspawner. How can we help to speed things up?

@willingc
Copy link
Contributor

I'm adding this issue to the September sprint to verify if we have what is needed for an updated release.

@rkdarst
Copy link
Contributor

rkdarst commented Aug 26, 2019

We've got more than enough for a release, it seems that the main maintainers don't have time. I'd be happy to help make a release, but that doesn't help much if we still can't react to and fix issues fast enough.

I'll also be at that September sprint and hopefully we can see what to do. I'll try to pass through all issues but I don't think there is much (besides subtle ProfileSpawner interactions) that's new.

@guillaumeeb, if there are currently important bugs for you, let me know and I can take a look and make an integration branch. There are some issues with profilespawner and spawner creation/options which I would like to discuss in more detail at the sprint before trying to fix.

@rkdarst
Copy link
Contributor

rkdarst commented Sep 6, 2019

Note on past release plan/future plans from the sprint: #138 (comment)

@willingc
Copy link
Contributor

willingc commented Sep 6, 2019

My proposal is to create a 0.9 release with functionality added through today.

A future 1.0 release could incorporate the new directions/features from the HPC sprint earlier this year.

Steps for Release

  • Review HPC sprint note
  • Update README
  • Update CHANGELOG
    ...

@willingc willingc added this to the 0.9 release milestone Sep 6, 2019
@mbmilligan
Copy link
Member Author

Thanks @willingc ! As @rkdarst says, we have a lot of accumulated work since the last release. I merged his last integration branch in July, so from my perspective I'm hoping to sort out if we are in a currently-good state for a release, or if there's additional changes of substance needed.

Incidentally, this is one of the reasons I'm hoping we can get some kind of regular (monthly?) check-in going among the Batchspawner folks. It would be so much easier to coordinate releasing if we could take five minutes to go around a group and get thumbs-up/down.

@jhamman
Copy link

jhamman commented Oct 8, 2019

My proposal is to create a 0.9 release with functionality added through today.

Hi folks. Based on @willingc's last post, it looks like things are quite close to a release. Is there anything that could use a little bump to make that happen in the near term?

@rcthomas
Copy link
Contributor

rcthomas commented Oct 8, 2019

@jhamman what we need to do here is some testing at various sites; that would increase confidence in a release. At NERSC we're running off an old commit because of our own previous technical debt and I've promised @mbmilligan that I'll clear that this month and get a test deployment going off batchspawner master, hopefully so we can switch to it for production around Oct 25.

@jhamman
Copy link

jhamman commented Oct 8, 2019

Would you consider making a pre-release now then? We would be much more capable of contributing to a live testing while using a release candidate.

@rcthomas
Copy link
Contributor

rcthomas commented Oct 23, 2019

@rkdarst I'm trying to run off both batchspawner and wrapspawner master, I have a custom wrapspawner class. I confirm the port is being sent from the remote side via the API, and I have import batchspawner in my config. I added some logging to my batchspawner-singleuser to confirm I get the message back that it has been received and I see the POST succeed in the logs. I've added some logging to the while self.port == 0 loop but the port stays 0 even though the call to the API has been made. Am I missing something?

@rcthomas
Copy link
Contributor

I confirm the parent class port is set, the child class port is not updated. With logging/print debugging I see the directional link being set. It just doesn't work...?

@rcthomas
Copy link
Contributor

@rkdarst @mbmilligan I traced the issue back to something to do with named servers, not with wrapspawner. I do think #167 addresses it.

@mbmilligan
Copy link
Member Author

Okay I've merged #167 - I think that was the last blocking issue we were aware of? If there are no objections, I'll tag an RC release in preparation for issuing a real release.

@casparvl
Copy link

Not sure what the status of the release is, but since we wanted to use the prologue functionality to load some modules before launching the notebook, I have tried running off the master branch.

It works now, but right after upgrading I did have some issues. For 0.8.1, in our config, we had specified (among other things):

c.BatchSpawnerBase.batch_query_cmd = "/usr/bin/squeue -h -j {job_id} -o '%T %B'"
c.BatchSpawnerBase.batch_submit_cmd = "sudo -E /usr/bin/sbatch --parsable --uid {username}"
c.BatchSpawnerBase.batch_cancel_cmd = "sudo /usr/bin/scancel {job_id}"

Now, with the current master I suddenly got a submit command:

sudo -E -u sudo -E /usr/bin/sbatch --parsable --uid {username}

Looking at the sources, I see that batchspawner now has a traitlet exec_prefix which defaults to sudo -E -u and which got prepended to my own defined batch command.

Similarly, I had a
c.Spawner.args="-ip=0.0.0.0"
In my old config. After upgrading batchspawner, the launchcommand now included the -ip=0.0.0.0 argument twice: once added by my config, and once added by the new version of batchspawner.

I just wanted to make you aware of this, because these changes in default behaviour means that upgrading from 0.8.1 to 0.9 will 'break' the installation for people like me, and will require users to change their configs to work with 0.9. Now, I'm not saying the changes in defaults are not justified (they make a lot of sense), but you might want to make a very clear release notes section so that users who upgrade know what setting to look out for / change in order to make things work again.

@mbmilligan
Copy link
Member Author

After an extensive period of development, I have tagged version 1.0.0 and made a corresponding release. Thank you everyone for your help.

As I expect a period of rapid catch-up development to follow, users desiring a stable batchspawner should probably pull from this tag rather than the main branch for the next little while.

Closing thread.

@cmd-ntrf
Copy link
Contributor

@mbmilligan : are you planning to also release it on pypi?

@mbmilligan
Copy link
Member Author

Yes. It'll take me a couple of days to figure out where I left the tooling for doing that, so that's about how long people have to discover terrible flaws and call for a bugfix release first.

@rkdarst
Copy link
Contributor

rkdarst commented Jul 21, 2020 via email

@mbmilligan
Copy link
Member Author

I'd try (if I could) to set up Github Actions to automatically off of tags. I know if should be simple, but I've never done it before
myself.

That would be excellent if you could figure that out. Last time I looked at it I think I stalled out on understanding how to get the PyPI credentials hooked up correctly, but I think Github Actions are significantly more featureful now than last time too. But I've seen blog posts about triggering those uploads off of Github Release tags so it should definitely be possible now, and there ought to be recipes out there.

@cmd-ntrf
Copy link
Contributor

I recently did the GitHub actions workflow to release wheel to PyPi for jupyter-lmod.
There is a template to do it in GitHub actions. Here is also an example:
https://github.com/cmd-ntrf/jupyter-lmod/blob/master/.github/workflows/main.yml

You then have to define your pypi username and password in the Secrets section of the repo Settings as PYPI_USERNAME and PYPI_PASSWORD. Then next time you make a GitHub release, GitHub action will publish the wheel and the source on PyPI.

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

No branches or pull requests

8 participants