-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Tool Shed 2.0 #15639
Tool Shed 2.0 #15639
Conversation
b69d768
to
ad7b6b0
Compare
This is a piece that is needed to create a schema package - which I would really like to have around for galaxyproject#15639 and to in general write higher-level API tests.
235f27e
to
82086d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the instructions outlined in the PR description, I ran into two errors which prevented me from running the toolshed locally:
The frontend threw an error which I detailed in the review comment for [...]/frontend/package.json
The backend threw the following error, causing a worker to be stuck in a restart loop:
Traceback (most recent call last):
File "/home/lailalos/Projects/galaxy/lib/tool_shed/webapp/buildapp.py", line 91, in app_pair
app = tool_shed.webapp.app.UniverseApplication(global_conf=global_conf, **kwargs)
File "/home/lailalos/Projects/galaxy/lib/tool_shed/webapp/app.py", line 56, in __init__
self.config: Any = config.Configuration(**kwd)
File "/home/lailalos/Projects/galaxy/lib/tool_shed/webapp/config.py", line 41, in __init__
self._process_config(kwargs)
File "/home/lailalos/Projects/galaxy/lib/tool_shed/webapp/config.py", line 136, in _process_config
self.password_expiration_period = timedelta(days=int(self.password_expiration_period))
AttributeError: 'ToolShedAppConfiguration' object has no attribute 'password_expiration_period'
I tried creating a tool_shed.yml
from the provided sample and setting password_expiration_period
to 0
, which did not fix the issue.
"scripts": { | ||
"dev": "vite --port 4040 --strict-port", | ||
"build": "vue-tsc --noEmit && vite build", | ||
"graphql": "graphql-codegen --watch", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running this either directly, or through dev all
, throws the following error:
Failed to load schema from http://localhost:9009/graphql/:
connect ECONNREFUSED ::1:9009
Error: connect ECONNREFUSED ::1:9009
at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1494:16)
GraphQL Code Generator supports:
- ES Modules and CommonJS exports (export as default or named export "schema")
- Introspection JSON File
- URL of GraphQL endpoint
- Multiple files with type definitions (glob expression)
- String in config file
Try to use one of above options and run codegen again.
Steps from clean slate:
- dependencies installed with
yarn
- ran
make dev
6f991ad
to
276b5d0
Compare
The toolshed backend error was caused by a messed up rebase and I think I've fixed it now. The backend serves as the GraphQL server the frontend needs to generate code... so that is why dev code doesn't work... it needs a GraphQL server to monitor for model changes. Maybe it is worth exploring if a vite server without the model watching for GraphQL is viable... it wouldn't be as full reactive to dev changes but it would be sufficient for all sorts of development. |
self._repo_install(changeset="1") | ||
version1 = tool_shed_install.ToolVersion() | ||
version1.tool_id = "github.com/galaxyproject/example/test_tool/0.1" | ||
self.app.install_model.context.add(version1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the context
for backward compatibility? Given that context
here is the session, do you want to switch to calling it session
(or sa_session, etc.) for consistency? (i.e., self.app.install_model.session
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that comment meant context.current was the backward compatible thing. I'm fine with install_model.session but it hasn't been my go to for tens of thousands of lines of code since that comment 😅. We should document a best practice here maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like session
(or sa_sesssion
). I think session
is a slightly more accurate term for what the object is; even with the occasional presence of galaxy_session
, I think context
is more generic and may be harder to immediately recognize as the identifier for SQLAlchemy's session. But I'm +100 on documenting best practices (naming key objects like Session, as well as how to handle typical db access tasks, etc.)
if deleted and not trans.user_is_admin: | ||
raise exceptions.AdminRequiredException("Only administrators can query deleted categories.") | ||
for category in ( | ||
trans.sa_session.query(Category).filter(Category.table.c.deleted == deleted).order_by(Category.table.c.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trans.sa_session.query(Category).filter(Category.table.c.deleted == deleted).order_by(Category.table.c.name) | |
trans.sa_session.scalars(select(Category).where(Category.deleted == deleted).order_by(Category.name)) |
…tions... ... so we can get the typing right and not depend on app.name.
…ions. Stronger typing and flatter functions
…d ts versions. Avoid all the big conditionals in the middle - again for stronger typing and flatter functions.
Goals: - Replace direct database access with pydantic typed API requests. - Replace Twill with playwright. - Eliminate access to the Galaxy UI in the tests - the tests should assume API access from the installing party. pytest-playwright requires small bits of galaxyproject#13909. This is a worthy project that could let us remove a bunch of deprecated stuff from the Galaxy admin controllers and a bunch of mako stuff that is unused and could be used to test PRs like galaxyproject#14609 (which prompted me to do this) but I'm anxious about growing emotionally attached to code I want to remove and I'm worried about losing track of which helpers are required for Planemo/Emphemeris/Galaxy and which helpers are just being used to test the tool shed.
app.model.Repository.table.c.user_id == app.model.User.table.c.id, | ||
) | ||
] | ||
repository = app.model.context.query(app.model.Repository).filter(*clause_list).first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repository = app.model.context.query(app.model.Repository).filter(*clause_list).first() | |
repository = app.model.context.scalars(select(app.model.Repository).where(*clause_list).limit(1)).first() |
app.model.Repository.table.c.deprecated == false(), | ||
app.model.Repository.table.c.deleted == false(), | ||
app.model.Repository.table.c.name == name, | ||
app.model.User.table.c.username == owner, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.model.Repository.table.c.deprecated == false(), | |
app.model.Repository.table.c.deleted == false(), | |
app.model.Repository.table.c.name == name, | |
app.model.User.table.c.username == owner, | |
app.model.Repository.deprecated == false(), | |
app.model.Repository.deleted == false(), | |
app.model.Repository.name == name, | |
app.model.User.username == owner, |
app.model.Repository.table.c.deleted == false(), | ||
app.model.Repository.table.c.name == name, | ||
app.model.User.table.c.username == owner, | ||
app.model.Repository.table.c.user_id == app.model.User.table.c.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.model.Repository.table.c.user_id == app.model.User.table.c.id, | |
app.model.Repository.user_id == app.model.User.id, |
276b5d0
to
50f787a
Compare
I'll wait to go over the SQLAlchemy syntax until you move it out of review. |
50f787a
to
9a292d6
Compare
Such a chicken and egg thing here... they will be further divergent from the other code that you also wanted to update that brought you here. I'll collect your suggestion into a commit though! I appreciate the feedback. A fun winter break project becoming the whole project's chore is just such a John thing 😆 - looking at you planemo and pulsar. |
LOL :) Only reason: I thought you were force-pushing stuff - I was afraid my suggestions would be immediately outdated. I'll go over it today. |
9a292d6
to
d3b434c
Compare
It'll be easier to fix all syntax issues in a follow-up PR: too much of the same cut and paste, and my browser is literally stalling (I suspect it's the 23K-lines editable change set it's trying to display). It's all under |
self._repo_install(changeset="1") | ||
version1 = tool_shed_install.ToolVersion() | ||
version1.tool_id = "github.com/galaxyproject/example/test_tool/0.1" | ||
self.app.install_model.context.add(version1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like session
(or sa_sesssion
). I think session
is a slightly more accurate term for what the object is; even with the occasional presence of galaxy_session
, I think context
is more generic and may be harder to immediately recognize as the identifier for SQLAlchemy's session. But I'm +100 on documenting best practices (naming key objects like Session, as well as how to handle typical db access tasks, etc.)
app.model.Repository.table.c.deprecated == false(), | ||
app.model.Repository.table.c.deleted == false(), | ||
app.model.Repository.table.c.name == name, | ||
app.model.User.table.c.username == owner, | ||
app.model.Repository.table.c.user_id == app.model.User.table.c.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: app.model.Foo.table.c.bar
>> app.model.Foo.bar
app.model.Repository.table.c.user_id == app.model.User.table.c.id, | ||
) | ||
] | ||
repository = app.model.context.current.sa_session.query(app.model.Repository).filter(*clause_list).first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repository = app.model.context.current.sa_session.query(app.model.Repository).filter(*clause_list).first() | |
repository = app.model.context.current.sa_session.scalars(app.model.Repository).where(*clause_list).limit(1)).first() |
app.model.Repository.table.c.deprecated == false(), | ||
app.model.Repository.table.c.deleted == deleted, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above: drop table.c.
app.model.User.table.c.username == owner, | ||
app.model.Repository.table.c.user_id == app.model.User.table.c.id, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same: table.c.
repository = ( | ||
self.sa_session.query(self.app.model.Repository) | ||
.filter( | ||
and_( | ||
self.app.model.Repository.table.c.name == name, | ||
self.app.model.Repository.table.c.user_id == user.id, | ||
) | ||
) | ||
.one() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repository = ( | |
self.sa_session.query(self.app.model.Repository) | |
.filter( | |
and_( | |
self.app.model.Repository.table.c.name == name, | |
self.app.model.Repository.table.c.user_id == user.id, | |
) | |
) | |
.one() | |
) | |
repository = ( | |
self.sa_session.execute(select(self.app.model.Repository) | |
.where( | |
and_( | |
self.app.model.Repository.name == name, | |
self.app.model.Repository.user_id == user.id, | |
) | |
) | |
).scalar_one() | |
) |
formatting will be off here (my browser is barely handling this inline editing..)
session.query(galaxy.model.tool_shed_install.ToolShedRepository) | ||
.filter( | ||
and_( | ||
galaxy.model.tool_shed_install.ToolShedRepository.table.c.deleted == false(), | ||
galaxy.model.tool_shed_install.ToolShedRepository.table.c.uninstalled == false(), | ||
galaxy.model.tool_shed_install.ToolShedRepository.table.c.status | ||
== galaxy.model.tool_shed_install.ToolShedRepository.installation_status.INSTALLED, | ||
) | ||
.all() | ||
) | ||
else: | ||
return install_session().query(galaxy.model.tool_shed_install.ToolShedRepository).all() | ||
.all() | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stmt = select(ToolShedRepository).where(and_(ToolShedRepository.deleted == false(), ToolShedRepository.uninstalled == false(), ToolShedRepository.stats == ToolShedRepository.installation_status.INSTALLED)
return session.scalars(stmt).all()
Also, no need to call list()
: all()
produces a list.
def get_installed_repository_by_name_owner(repository_name, owner, return_multiple=False, session=None): | ||
if session is None: | ||
session = install_session() | ||
query = session.query(galaxy.model.tool_shed_install.ToolShedRepository).filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convert to select
construct? Then, in the return clause, just session.scalars(stmt).all()
for multiple and
session.scalars(stmt.limit(1)).first()
for one (note the limit clause).
session = install_session() | ||
query = session.query(galaxy.model.tool_shed_install.ToolShedRepository).filter( | ||
and_( | ||
galaxy.model.tool_shed_install.ToolShedRepository.table.c.name == repository_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
table.c.
(same as above)
) -> Optional[Dict[str, Any]]: | ||
clause_list = [] | ||
if name is not None: | ||
clause_list.append(galaxy_model.ToolShedRepository.table.c.name == name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.table.c
d3b434c
to
ec6d8fd
Compare
The test ToolShed https://testtoolshed.g2.bx.psu.edu/ has been updated after galaxyproject/galaxy#15639 was merged.
The test ToolShed https://testtoolshed.g2.bx.psu.edu/ has been updated after galaxyproject/galaxy#15639 was merged.
The test ToolShed https://testtoolshed.g2.bx.psu.edu/ has been updated after galaxyproject/galaxy#15639 was merged.
Goals
The goal here is to introduce a new tool shed that breaks nothing but cuts all sorts of ties to old technologies and old concepts that we’re no longer interested in supporting.
Technology Choices
At its core, these technologies mirror the directions Galaxy has moved over the last decade but in some cases there is small divergence. The long term goal of stripping the tool shed out of the Galaxy code base remains firmly intact. The tool shed now has its own client build process and such and doesn’t depend on any shared mako or Javascript glue with Galaxy client - and I hope this allows for that freedom to make some small different choices at the top of the client stack. In particular, the tool shed is very small (especially now) compared Galaxy so the freedom and flexibility and configurability of say Bootstrap and WebPack that are important for large projects with significant legacy features are probably not as important as the rapid prototyping and more on the rails experience (both visually and in terms of developer workflow) of say Quasar, Material, and Vite.
GraphQL remains an interesting experiment to my mind. I’ve done the hard work here of figuring out how to integrate it with a Galaxy-like app (both in terms of the backend and frontend). I don’t think it should be thrown out and I think it should remain in the tool shed - I’m somewhat more ambivalent about whether I’d use it in Galaxy or if I would keep in a second cleanroom rewrite of the Tool Shed. What is here is a good and successful experiment though I think and remains valuable data.
A Non-exhaustive List of Things I’ve Removed from the Frontend
Developing the Tool Shed
The TypeScript GraphQL development tools require a GraphQL server to query for typing details and such. So the backend of the tool shed must be running before the frontend development tools can be launched (at least the GraphQL ones).
Backend Runtime
The backend (along with a GraphQL console) can be started with the following command:
If that API key is set as that, the following script will bootstrap the new tool shed against the main tool shed to have some things to play with.
Frontend Runtime:
make dev
(from the frontend directory) oryarn dev-all
from that same directory will run a Vite development server for the frontend (kind of the equivalent of webpack watch) and it will monitor the frontend code for GraphQL queries and generated typed TypeScript artifacts against the the backend server.The one thing that isn’t covered by that - is TypeScript fetcher stuff for the API. I’ve piggy-backed on the Galaxy Makefile task for this - so make update-client-api-schema from Galaxy’s Makefile will generate these.
Production:
Run
make client
from the frontend script to build artifacts needed to for deploying the frontend.Frontend Linting:
yarn
andMakefile
targets for linting and running prettier are included.make lint
will check both of those as well as type checking. "make format” will format the code.How to test the changes?
(Select all options that apply)
License