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

update README #515

Closed
wants to merge 10 commits into from
Closed

update README #515

wants to merge 10 commits into from

Conversation

will-moore
Copy link
Member

Issues with Docker-based dev instructions at https://forum.image.sc/t/omero-figure-dev-install-with-docker-fails/86022
prompted me to tidy them (fix formatting) and add the option of local install.

NB: However, as noted on that issue, the instructions won't work because openmicroscopy/omero-web-standalone:latest Docker image is still python 3.6, which forces Django 3.2 to be installed, which is incompatible (in development mode) with ome/omero-web#480 which was included in omero-web 5.22.0.

Can try to use older omero-web in Dockerfile...

FROM openmicroscopy/omero-web-standalone:5.21.0

Django 3.6 will be installed as the lastest version that works on python2.6
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/omero-figure-dev-install-with-docker-fails/86022/5

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#47. See the console output for more details.
Possible conflicts:

--conflicts

README.rst Outdated Show resolved Hide resolved
README.rst Outdated


To develop ``omero-figure`` you can either make all the changes within the Docker container itself by running:
::


$ docker run -ti -e OMEROHOST=YOUR_HOST -p 4080:4080 figure-devel
$ docker run -ti -e OMEROHOST=$HOST -p 4080:4080 figure-devel
Copy link
Member

Choose a reason for hiding this comment

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

If this is defined as $OMEROHOST above, then you only need:

...-ti -e OMEROHOST -p ...

@snoopycrimecop
Copy link
Member

snoopycrimecop commented Sep 11, 2023

Conflicting PR. Removed from build OMERO-plugins-push#49. See the console output for more details.
Possible conflicts:

--conflicts Conflict resolved in build OMERO-plugins-push#252. See the console output for more details.

@will-moore will-moore mentioned this pull request Sep 12, 2023
@will-moore will-moore added this to the 6.1.0 milestone Nov 2, 2023
@will-moore
Copy link
Member Author

@jburel - I can't seem to reproduce the original error reported above, so I've reverted the Dockerfile change (maybe the omero-web-standalone got updated already)?
Otherwise, just updates to the README...

@jburel
Copy link
Member

jburel commented Dec 8, 2023

@will-moore I have merged the omeroweb-docker so it is now running Python 3.9

README.rst Show resolved Hide resolved
README.rst Show resolved Hide resolved
Copy link
Member

@pwalczysko pwalczysko left a comment

Choose a reason for hiding this comment

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

Please address #515 (comment)

@pwalczysko pwalczysko mentioned this pull request Dec 13, 2023
@will-moore
Copy link
Member Author

Hmm - OK, now I'm confused....

The error you're seeing is what's described in the PR description above. And I worked around it at the time with FROM openmicroscopy/omero-web-standalone:5.21.0

But then I removed that change (to try and check the error again) and didn't see an error (maybe the Docker Image was cached and removing the change had no effect).

And now @jburel comment #515 (comment) implies that we don't need the workaround any more as the omero-web standalone docker is now up-to-date (python 3.9)??

@jburel
Copy link
Member

jburel commented Dec 13, 2023

I have merged the omero-web-docker PR but not tagged the repository so the new docker image is not available on dockerhub. I was waiting for the next release of omero-web
We could do a tag if it is urgent

@will-moore
Copy link
Member Author

I have reverted to use FROM openmicroscopy/omero-web-standalone:5.21.0 which should fix that error until omero-web-standalone is updated.

@pwalczysko
Copy link
Member

I have reverted to use FROM openmicroscopy/omero-web-standalone:5.21.0 which should fix that error until omero-web-standalone is updated.

But some problem is then detected when following the workflow in the browser, see https://github.com/ome/omero-figure/pull/515/files#r1426873071 (not claiming that it was not there before, just that the README does somehow not work as written)

README.rst Outdated
Using Docker
************

It is also possible to develop figure in docker without installing anything locally.
Copy link
Member

@pwalczysko pwalczysko Dec 15, 2023

Choose a reason for hiding this comment

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

This is obviously not true. After consultaion, I understand that you have to install both npm and grunt for one of the workflows below (for the recommended one in fact).

Copy link
Member Author

Choose a reason for hiding this comment

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

Tweaked this in f35ae2e

README.rst Outdated
$ docker build -t figure-devel .
$ export OMEROHOST=demo.openmicroscopy.org

The preferred option is to mount the repository containing the omero-figure code. You can make the changes locally and run `grunt watch`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The preferred option is to mount the repository containing the omero-figure code. You can make the changes locally and run `grunt watch`
The preferred option is to mount the repository containing the omero-figure code. For this, you need to install locally ``npm`` and ``grunt`` as described above. You can make the changes locally and run `grunt watch`

README.rst Outdated
Using Docker
************

It is also possible to develop figure in docker without installing anything locally.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
It is also possible to develop figure in docker without installing anything locally.
It is also possible to develop figure in docker without installing omero-web or omero-figure locally.

@pwalczysko
Copy link
Member

Failed checks.

@will-moore could you maybe read my suggestions in the text and accept or reformulate, then commit and we can read afresh on Monday ? I do not see any probs anymore with the workflow (although the failed checks are worrying) - it is just about formulating the README.

@pwalczysko pwalczysko self-requested a review December 16, 2023 16:38
Copy link
Member

@pwalczysko pwalczysko left a comment

Choose a reason for hiding this comment

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

Have a look at the suggested reformulations of the text
Check the failed checks on PR

@pwalczysko
Copy link
Member

The failing checks are connected to flake8 install imho https://github.com/ome/omero-figure/actions/runs/7264947038/job/19793566993 - the attempt to fix them by upgrading the testing framework in #535 have failed - the flake8 error persists. Any ideas please @joshmoore @khaledk2 ?

@jburel
Copy link
Member

jburel commented Dec 19, 2023

@pwalczysko The requirements in omero-test-infra will need to be updated see https://github.com/ome/omero-test-infra/blob/master/requirements.txt.
That also means that omero-figure will need to be adjusted. Possibly some new flake8 failure e.g. usage of parameter like l is not permitted anymore
Some Javascripts failure might occur too.

I will not recommend to upgrade omero-test-infra before the break since many repositories will be affected (including external repo like ezomero)

One option will be to work from a personal branch to get omero-figure green via the omero_plugin.yml

@pwalczysko
Copy link
Member

One option will be to work from a personal branch to get omero-figure green via the omero_plugin.yml

Meaning pointing inside omero_plugin.yml to a personal branch on the https://github.com/ome/omero-test-infra/blob/master/requirements.txt repo ?
We discussed on Standup and will sort this out in the New Year.

@pwalczysko pwalczysko mentioned this pull request Dec 20, 2023
@will-moore
Copy link
Member Author

@jburel @pwalczysko Thanks for your help.
Added all your commits here - Tests of Figure export script are failing - don't know why just now, but will look again in the New Year!

@jburel
Copy link
Member

jburel commented Jan 3, 2024

@will-moore could you remove the commit not related to the README update?
The commits related to the upgrade to test-infra are included in #537

@will-moore
Copy link
Member Author

@jburel Done. Just the README and DockerFile changed now.

@jburel
Copy link
Member

jburel commented Jan 3, 2024

The latest docker image has now been updated to Rocky Linux 9 so Python version will now be 3.10

@@ -1,4 +1,4 @@
FROM openmicroscopy/omero-web-standalone:latest
FROM openmicroscopy/omero-web-standalone:5.21.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FROM openmicroscopy/omero-web-standalone:5.21.0
FROM openmicroscopy/omero-web-standalone:latest

as per @jburel's comment - this works fine for me

Copy link
Member

@pwalczysko pwalczysko Jan 3, 2024

Choose a reason for hiding this comment

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

this works fine for me

Sorry, have to eat my words. It works fine only up to a point. This means that the docker image builds fine, and the docker starts as expected. I am able to connect to demo server. But then:

  • if using the latest image, I am not able to watch the changes live as described in this README, for example changing the index.html file and renaming the Export PDF button does not appear immediately after refresh
  • if using the 5.21.0 image, I am able to watch the changes live, ie. the Export PDF button manipulation in the index.html is immediately visible after refresh of the browser

Adding -e CONFIG_omero_web_debug=true to the docker run... cmd with the latest build image does not help, still no changes visible after refresh. Note that the changes are visible if you stop the docker and run the docker run... cmd again - then it works even with the latest image

README.rst Outdated Show resolved Hide resolved
README.rst Outdated
@@ -208,6 +208,8 @@ You can refresh the Docker hosted page at http://localhost:4080/figure to see ch
$ cd /PATH_TO_GIT_REPO/omero-figure
$ grunt build
$ grunt watch # to see updates

# in a different terminal...
Copy link
Member

Choose a reason for hiding this comment

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

In the cmd below you are using the OMEROHOST variable which was defined in the previous terminal. Maybe move the OMEROHOST definition to here instead ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@pwalczysko instead, I switched the order you launch these so that grunt is in a different terminal and you launch docker first in the terminal that does have OMEROHOST - see a6940e1

will-moore added a commit to will-moore/figure that referenced this pull request Jan 19, 2024
@will-moore will-moore removed this from the 6.1.0 milestone Feb 15, 2024
@will-moore
Copy link
Member Author

Decided that we don't want to keep supporting Docker for omero-web development. Takes too much testing (as above) and maintenance.

@will-moore will-moore closed this Feb 26, 2024
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